)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"d16bf21b5758782ff39d49f681679a02be7f01d1","unresolved":false,"context_lines":[{"line_number":12,"context_line":"rebalance much faster on dense chassis with many devices."},{"line_number":13,"context_line":""},{"line_number":14,"context_line":"Currently the default is still only one process, and no workers.  Set"},{"line_number":15,"context_line":"reconstructor_workers in the [object-reconstrcutor] section to some"},{"line_number":16,"context_line":"whole number \u003c\u003d the number of devices on a node to get that many"},{"line_number":17,"context_line":"reconstructor workers."},{"line_number":18,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"3f1d235d_0527c766","line":15,"range":{"start_line":15,"start_character":37,"end_line":15,"end_character":50},"updated":"2017-07-10 18:40:25.000000000","message":"nit: reconstructor","commit_id":"9f3e7cf1ed98ad65d57295ebe51260280054aa72"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"16cecc0cae0846a86e74a51e48d3037b7efa7c30","unresolved":false,"context_lines":[{"line_number":12,"context_line":"rebalance much faster on dense chassis with many devices."},{"line_number":13,"context_line":""},{"line_number":14,"context_line":"Currently the default is still only one process, and no workers.  Set"},{"line_number":15,"context_line":"reconstructor_workers in the [object-reconstrcutor] section to some"},{"line_number":16,"context_line":"whole number \u003c\u003d the number of devices on a node to get that many"},{"line_number":17,"context_line":"reconstructor workers."},{"line_number":18,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"3f1d235d_c5c66817","line":15,"in_reply_to":"3f1d235d_0527c766","updated":"2017-07-11 00:19:56.000000000","message":"Done","commit_id":"9f3e7cf1ed98ad65d57295ebe51260280054aa72"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"a9c09b33d0d04dee00919fa16a88ef3fa837078c","unresolved":false,"context_lines":[{"line_number":32,"context_line":"every stats_interval (default 5 mins)."},{"line_number":33,"context_line":""},{"line_number":34,"context_line":"reconstructor_workers is not documented outside of the example config"},{"line_number":35,"context_line":"Related-Bug #1626290"},{"line_number":36,"context_line":""},{"line_number":37,"context_line":"Change-Id: I28925a37f3985c9082b5a06e76af4dc3ec813abe"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":8,"id":"1f1a1f67_6bdb1c4f","line":35,"updated":"2017-07-20 13:41:50.000000000","message":"independent fix for this proposed here https://review.openstack.org/#/c/485569/","commit_id":"52d92732cd19f73893f04738a89d8d9eaa2df4fd"}],"swift/common/daemon.py":[{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"1ff882b8599733c9d7cb831fe0ddc29fec812c6f","unresolved":false,"context_lines":[{"line_number":47,"context_line":"            self.run_forever(**kwargs)"},{"line_number":48,"context_line":""},{"line_number":49,"context_line":""},{"line_number":50,"context_line":"class WorkerDaemonMixin(object):"},{"line_number":51,"context_line":"    \"\"\""},{"line_number":52,"context_line":"    Mixin to describe nessecary interface for a daemon to be compatible with"},{"line_number":53,"context_line":"    the WorkerStrategy."}],"source_content_type":"text/x-python","patch_set":1,"id":"3f1d235d_59281185","line":50,"updated":"2017-06-30 14:37:15.000000000","message":"Why a mixin? The only use case we have is inheriting into a Daemon subclass, so why not make this a subclass of Daemon (MultiWorkersDaemon?) and then have ObjectReconstructor inherit from that? The mixin identifies an object that is to be used with a WorkersStrategy, but that object must also be of type Daemon.","commit_id":"18a394fff8aac3a40e4b70a9629cedf0db474d55"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"1ff882b8599733c9d7cb831fe0ddc29fec812c6f","unresolved":false,"context_lines":[{"line_number":74,"context_line":"        \"\"\""},{"line_number":75,"context_line":"        return [kwargs for i in range(self.workers)]"},{"line_number":76,"context_line":""},{"line_number":77,"context_line":"    def needs_to_restart_workers(self):"},{"line_number":78,"context_line":"        \"\"\""},{"line_number":79,"context_line":"        Currently we only support KILL EM ALL and restart EVERYTHING, an"},{"line_number":80,"context_line":"        implementation will need this method to return true if it wants to"}],"source_content_type":"text/x-python","patch_set":1,"id":"3f1d235d_91ab1d1c","line":77,"updated":"2017-06-30 14:37:15.000000000","message":"ok, so this might give opportunity for example for a daemon to notice that some workers are much busier than others and reasign workload??","commit_id":"18a394fff8aac3a40e4b70a9629cedf0db474d55"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"1ff882b8599733c9d7cb831fe0ddc29fec812c6f","unresolved":false,"context_lines":[{"line_number":86,"context_line":"        return False"},{"line_number":87,"context_line":""},{"line_number":88,"context_line":""},{"line_number":89,"context_line":"class DaemonStrategy(object):"},{"line_number":90,"context_line":""},{"line_number":91,"context_line":"    def __init__(self, daemon, logger):"},{"line_number":92,"context_line":"        self.daemon \u003d daemon"}],"source_content_type":"text/x-python","patch_set":1,"id":"3f1d235d_dfa2c19a","line":89,"updated":"2017-06-30 14:37:15.000000000","message":"I can see the value of the strategy pattern when the same strategy is to be applied to different types, or a strategy is chosen dynamically for a type, but here we have a fixed 1-1 mapping between strategy and daemon type, so the strategy could be rolled into the daemon type.\n\n(Here, the strategy does dynamically change based on workers\u003c1 but that is done within the WorkersDaemonStrategy, not by selecting DaemonStrategy based on workers\u003c1; but that  is necessary because plain old Daemons don\u0027t have a workers attr. We could do:\n\n  if hasattr(daemon, workers) and daemon.workers \u003c 1:\n      strategy \u003d DaemonStrategy\netc etc\n\nbut it doesn\u0027t seem worth it.\n\nMaybe in future we\u0027ll invent other strategies and want to dynamically associate them with daemons, in which case decomposing them may make sense. But I wonder if for now it is easier to grok just a Daemon class and a WorkersDaemon subclass, no mixins or strategies.","commit_id":"18a394fff8aac3a40e4b70a9629cedf0db474d55"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"1ff882b8599733c9d7cb831fe0ddc29fec812c6f","unresolved":false,"context_lines":[{"line_number":200,"context_line":"        if self.daemon.workers \u003c 1:"},{"line_number":201,"context_line":"            return super(WorkersStrategy, self)._run(once, **kwargs)"},{"line_number":202,"context_line":"        self.ask_daemon_to_prepare_workers(**kwargs)"},{"line_number":203,"context_line":"        for per_worker_options in self.iter_unspawned_workers():"},{"line_number":204,"context_line":"            if self._fork(once, **per_worker_options) \u003d\u003d 0:"},{"line_number":205,"context_line":"                return 0"},{"line_number":206,"context_line":"        while self.running:"}],"source_content_type":"text/x-python","patch_set":1,"id":"3f1d235d_71e3e134","line":203,"range":{"start_line":203,"start_character":39,"end_line":203,"end_character":61},"updated":"2017-06-30 14:37:15.000000000","message":"the number of workers is actually determined by the length of this iter, which in the defult implementation is self.workers, but there is an opportunity for a daemon impl to screw up and not return \u0027workers\u0027 worth of kwargs - in fact, this would happen by design I guess when the reconstructor has devices \u003c workers*\n\nSimilarly could the daemon abuse this method and return more than \u0027workers\u0027 options, and we\u0027ll kick off that many children?? \n\nAlso, should the conf option be called max_reconstructor_workers - could an op be confused hunting for \u0027workers\u0027 child processes when there are only num_devices children?\n\n* (yes, confirmed with reconstructor_workers\u003d20 on my SAIO there\u0027s only 2 workers for 2 devices per object server)","commit_id":"18a394fff8aac3a40e4b70a9629cedf0db474d55"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"1ff882b8599733c9d7cb831fe0ddc29fec812c6f","unresolved":false,"context_lines":[{"line_number":209,"context_line":"            self.check_on_all_running_workers()"},{"line_number":210,"context_line":"            if not once:"},{"line_number":211,"context_line":"                for per_worker_options in self.iter_unspawned_workers():"},{"line_number":212,"context_line":"                    if self._fork(once, **per_worker_options) \u003d\u003d 0:"},{"line_number":213,"context_line":"                        return 0"},{"line_number":214,"context_line":"            else:"},{"line_number":215,"context_line":"                if not self.spawned_pids():"}],"source_content_type":"text/x-python","patch_set":1,"id":"3f1d235d_d43bc3ce","line":212,"updated":"2017-06-30 14:37:15.000000000","message":"ok, fork new child for each failed process re-using kwargs","commit_id":"18a394fff8aac3a40e4b70a9629cedf0db474d55"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"d16bf21b5758782ff39d49f681679a02be7f01d1","unresolved":false,"context_lines":[{"line_number":86,"context_line":"        \"\"\""},{"line_number":87,"context_line":"        For each worker spit out a (possibly empty) dict of kwargs to pass"},{"line_number":88,"context_line":"        along to the daemons run method after fork.  The length of elements"},{"line_number":89,"context_line":"        returned from this method will be the number of processes created."},{"line_number":90,"context_line":""},{"line_number":91,"context_line":"        :param kwargs: plumbed through via command line argparser"},{"line_number":92,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"3f1d235d_47e82715","line":89,"updated":"2017-07-10 18:40:25.000000000","message":"what should be expected from this method when workers\u003d0? An empty list? i.e. should it be safe to call this method when create_workers is zero. I\u0027m thinking in terms of future maintenance, if there are multiple implementations of this interface and then we are changing the strategy, should we pin down now what behaviour is expected when workers\u003d0?","commit_id":"9f3e7cf1ed98ad65d57295ebe51260280054aa72"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"16cecc0cae0846a86e74a51e48d3037b7efa7c30","unresolved":false,"context_lines":[{"line_number":86,"context_line":"        \"\"\""},{"line_number":87,"context_line":"        For each worker spit out a (possibly empty) dict of kwargs to pass"},{"line_number":88,"context_line":"        along to the daemons run method after fork.  The length of elements"},{"line_number":89,"context_line":"        returned from this method will be the number of processes created."},{"line_number":90,"context_line":""},{"line_number":91,"context_line":"        :param kwargs: plumbed through via command line argparser"},{"line_number":92,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"3f1d235d_f04240ab","line":89,"in_reply_to":"3f1d235d_47e82715","updated":"2017-07-11 00:19:56.000000000","message":"Done","commit_id":"9f3e7cf1ed98ad65d57295ebe51260280054aa72"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"d16bf21b5758782ff39d49f681679a02be7f01d1","unresolved":false,"context_lines":[{"line_number":90,"context_line":""},{"line_number":91,"context_line":"        :param kwargs: plumbed through via command line argparser"},{"line_number":92,"context_line":""},{"line_number":93,"context_line":"        :returns: a list of dicts, each element represents the kwargs to be"},{"line_number":94,"context_line":"                  passed to a single workers\u0027s run method after fork."},{"line_number":95,"context_line":"        \"\"\""},{"line_number":96,"context_line":"        raise NotImplementedError()"}],"source_content_type":"text/x-python","patch_set":2,"id":"3f1d235d_c711d755","line":93,"range":{"start_line":93,"start_character":18,"end_line":93,"end_character":33},"updated":"2017-07-10 18:40:25.000000000","message":"nit: but the implementation returns a generator","commit_id":"9f3e7cf1ed98ad65d57295ebe51260280054aa72"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"16cecc0cae0846a86e74a51e48d3037b7efa7c30","unresolved":false,"context_lines":[{"line_number":90,"context_line":""},{"line_number":91,"context_line":"        :param kwargs: plumbed through via command line argparser"},{"line_number":92,"context_line":""},{"line_number":93,"context_line":"        :returns: a list of dicts, each element represents the kwargs to be"},{"line_number":94,"context_line":"                  passed to a single workers\u0027s run method after fork."},{"line_number":95,"context_line":"        \"\"\""},{"line_number":96,"context_line":"        raise NotImplementedError()"}],"source_content_type":"text/x-python","patch_set":2,"id":"3f1d235d_45da58e9","line":93,"in_reply_to":"3f1d235d_c711d755","updated":"2017-07-11 00:19:56.000000000","message":"Done","commit_id":"9f3e7cf1ed98ad65d57295ebe51260280054aa72"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"861dccd4c572ed6ebb3aa936a305b12aca24cf19","unresolved":false,"context_lines":[{"line_number":233,"context_line":"            self.register_worker_exit(p)"},{"line_number":234,"context_line":""},{"line_number":235,"context_line":"    def _run(self, once, **kwargs):"},{"line_number":236,"context_line":"        if not self.daemon.workers:"},{"line_number":237,"context_line":"            return super(WorkersStrategy, self)._run(once, **kwargs)"},{"line_number":238,"context_line":"        self.ask_daemon_to_prepare_workers(**kwargs)"},{"line_number":239,"context_line":"        for per_worker_options in self.iter_unspawned_workers():"}],"source_content_type":"text/x-python","patch_set":2,"id":"3f1d235d_daee73b3","line":236,"updated":"2017-07-08 01:27:09.000000000","message":"I keep waffling if it\u0027s easier for consumers of the interface to implement this property/attribute or if they should simply return an empty list/StopIteration in create_workers when then want to fall back to inline exectution\n\nsomething in \"import this\" makes me think this is more verbose but easier to grok...","commit_id":"9f3e7cf1ed98ad65d57295ebe51260280054aa72"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"16cecc0cae0846a86e74a51e48d3037b7efa7c30","unresolved":false,"context_lines":[{"line_number":233,"context_line":"            self.register_worker_exit(p)"},{"line_number":234,"context_line":""},{"line_number":235,"context_line":"    def _run(self, once, **kwargs):"},{"line_number":236,"context_line":"        if not self.daemon.workers:"},{"line_number":237,"context_line":"            return super(WorkersStrategy, self)._run(once, **kwargs)"},{"line_number":238,"context_line":"        self.ask_daemon_to_prepare_workers(**kwargs)"},{"line_number":239,"context_line":"        for per_worker_options in self.iter_unspawned_workers():"}],"source_content_type":"text/x-python","patch_set":2,"id":"3f1d235d_b01608a7","line":236,"in_reply_to":"3f1d235d_94066b20","updated":"2017-07-11 00:19:56.000000000","message":"Done","commit_id":"9f3e7cf1ed98ad65d57295ebe51260280054aa72"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"d16bf21b5758782ff39d49f681679a02be7f01d1","unresolved":false,"context_lines":[{"line_number":233,"context_line":"            self.register_worker_exit(p)"},{"line_number":234,"context_line":""},{"line_number":235,"context_line":"    def _run(self, once, **kwargs):"},{"line_number":236,"context_line":"        if not self.daemon.workers:"},{"line_number":237,"context_line":"            return super(WorkersStrategy, self)._run(once, **kwargs)"},{"line_number":238,"context_line":"        self.ask_daemon_to_prepare_workers(**kwargs)"},{"line_number":239,"context_line":"        for per_worker_options in self.iter_unspawned_workers():"}],"source_content_type":"text/x-python","patch_set":2,"id":"3f1d235d_94066b20","line":236,"in_reply_to":"3f1d235d_daee73b3","updated":"2017-07-10 18:40:25.000000000","message":"I\u0027m sure you considered this already, but how about:\n\n  worker_options \u003d self.daemon.create_workers(**kwargs)\n  if not worker_options:\n      return super(WorkersStrategy, self)._run(once, **kwargs)\n\n  self.unspawned_worker_options \u003d list(worker_options)\n\nI think that works even if create_worker is a generator","commit_id":"9f3e7cf1ed98ad65d57295ebe51260280054aa72"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"737b1413e75c300359d6dd886e7b0e848bef2ba1","unresolved":false,"context_lines":[{"line_number":34,"context_line":""},{"line_number":35,"context_line":"    A subclass of Daemon should implement run_once and run_forever."},{"line_number":36,"context_line":""},{"line_number":37,"context_line":"    Implementations of Daemon do not know *how* to daemonize, they simply"},{"line_number":38,"context_line":"    provide the behavior of the daemon."},{"line_number":39,"context_line":"    \"\"\""},{"line_number":40,"context_line":""},{"line_number":41,"context_line":"    def __init__(self, conf):"}],"source_content_type":"text/x-python","patch_set":5,"id":"1f1a1f67_25717eaa","line":38,"range":{"start_line":37,"start_character":4,"end_line":38,"end_character":39},"updated":"2017-07-14 19:50:11.000000000","message":"+1 useful comment","commit_id":"fcd24235da12b2bca8dbd550febe68723d593ba1"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"1dc83b5363aa66bc58f8f7c886e21df276f81588","unresolved":false,"context_lines":[{"line_number":35,"context_line":"    A subclass of Daemon should implement run_once and run_forever."},{"line_number":36,"context_line":""},{"line_number":37,"context_line":"    Implementations of Daemon do not know *how* to daemonize, they simply"},{"line_number":38,"context_line":"    provide the behavior of the daemon."},{"line_number":39,"context_line":"    \"\"\""},{"line_number":40,"context_line":""},{"line_number":41,"context_line":"    def __init__(self, conf):"}],"source_content_type":"text/x-python","patch_set":5,"id":"1f1a1f67_7d0fe736","line":38,"in_reply_to":"1f1a1f67_25717eaa","updated":"2017-07-17 05:15:23.000000000","message":"Done","commit_id":"fcd24235da12b2bca8dbd550febe68723d593ba1"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"737b1413e75c300359d6dd886e7b0e848bef2ba1","unresolved":false,"context_lines":[{"line_number":57,"context_line":"            self.run_forever(**kwargs)"},{"line_number":58,"context_line":""},{"line_number":59,"context_line":""},{"line_number":60,"context_line":"class WorkerDaemon(Daemon):"},{"line_number":61,"context_line":"    \"\"\""},{"line_number":62,"context_line":"    Expanded Daemon base class that supports the interface required by the"},{"line_number":63,"context_line":"    WorkersStrategy.  Implementations of the WorkerDaemon are Daemons and will"}],"source_content_type":"text/x-python","patch_set":5,"id":"1f1a1f67_13824850","line":60,"updated":"2017-07-14 19:50:11.000000000","message":"I have come to really like the separation of strategy and daemon. But I\u0027m still curious about the need for subclasses. If the two new methods defined implementations (create_workers returns empty list, is_healthy-\u003eTrue) then we could just have Daemon (and one strategy I think).\n\nIDK, I guess it helps to call out that there are interface extensions, but then we\u0027re not adding any properties in the subclass, not behaviour because the methods are not implemented.","commit_id":"fcd24235da12b2bca8dbd550febe68723d593ba1"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"737b1413e75c300359d6dd886e7b0e848bef2ba1","unresolved":false,"context_lines":[{"line_number":57,"context_line":"            self.run_forever(**kwargs)"},{"line_number":58,"context_line":""},{"line_number":59,"context_line":""},{"line_number":60,"context_line":"class WorkerDaemon(Daemon):"},{"line_number":61,"context_line":"    \"\"\""},{"line_number":62,"context_line":"    Expanded Daemon base class that supports the interface required by the"},{"line_number":63,"context_line":"    WorkersStrategy.  Implementations of the WorkerDaemon are Daemons and will"}],"source_content_type":"text/x-python","patch_set":5,"id":"1f1a1f67_53932050","line":60,"range":{"start_line":60,"start_character":6,"end_line":60,"end_character":18},"updated":"2017-07-14 19:50:11.000000000","message":"nit: if we stick with the subclass should it be WorkersDaemon (plural Worker*s*) to match WorkersStrategy\n\nhmmm, maybe that is deliberate - this class provides behaviour of *a* worker ... but it also provides create_workers which is to do with the plural.\n\n#super-nit","commit_id":"fcd24235da12b2bca8dbd550febe68723d593ba1"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"1dc83b5363aa66bc58f8f7c886e21df276f81588","unresolved":false,"context_lines":[{"line_number":57,"context_line":"            self.run_forever(**kwargs)"},{"line_number":58,"context_line":""},{"line_number":59,"context_line":""},{"line_number":60,"context_line":"class WorkerDaemon(Daemon):"},{"line_number":61,"context_line":"    \"\"\""},{"line_number":62,"context_line":"    Expanded Daemon base class that supports the interface required by the"},{"line_number":63,"context_line":"    WorkersStrategy.  Implementations of the WorkerDaemon are Daemons and will"}],"source_content_type":"text/x-python","patch_set":5,"id":"1f1a1f67_bd11ff5f","line":60,"in_reply_to":"1f1a1f67_53932050","updated":"2017-07-17 05:15:23.000000000","message":"Done","commit_id":"fcd24235da12b2bca8dbd550febe68723d593ba1"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"737b1413e75c300359d6dd886e7b0e848bef2ba1","unresolved":false,"context_lines":[{"line_number":62,"context_line":"    Expanded Daemon base class that supports the interface required by the"},{"line_number":63,"context_line":"    WorkersStrategy.  Implementations of the WorkerDaemon are Daemons and will"},{"line_number":64,"context_line":"    provide a run method which will be invoked in the worker process.  In"},{"line_number":65,"context_line":"    addition that provide the methods nessecary for the WorkersStrategy to"},{"line_number":66,"context_line":"    dispatch arguments to individual workers and perform context specific"},{"line_number":67,"context_line":"    periodic wellness checks."},{"line_number":68,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"1f1a1f67_e56a66b1","line":65,"range":{"start_line":65,"start_character":38,"end_line":65,"end_character":47},"updated":"2017-07-14 19:50:11.000000000","message":"s/nessecary/necessary/","commit_id":"fcd24235da12b2bca8dbd550febe68723d593ba1"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"1dc83b5363aa66bc58f8f7c886e21df276f81588","unresolved":false,"context_lines":[{"line_number":62,"context_line":"    Expanded Daemon base class that supports the interface required by the"},{"line_number":63,"context_line":"    WorkersStrategy.  Implementations of the WorkerDaemon are Daemons and will"},{"line_number":64,"context_line":"    provide a run method which will be invoked in the worker process.  In"},{"line_number":65,"context_line":"    addition that provide the methods nessecary for the WorkersStrategy to"},{"line_number":66,"context_line":"    dispatch arguments to individual workers and perform context specific"},{"line_number":67,"context_line":"    periodic wellness checks."},{"line_number":68,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"1f1a1f67_fd1bf77d","line":65,"in_reply_to":"1f1a1f67_e56a66b1","updated":"2017-07-17 05:15:23.000000000","message":"Done","commit_id":"fcd24235da12b2bca8dbd550febe68723d593ba1"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"737b1413e75c300359d6dd886e7b0e848bef2ba1","unresolved":false,"context_lines":[{"line_number":71,"context_line":"    specific knowledge about which workers should be doing what."},{"line_number":72,"context_line":"    \"\"\""},{"line_number":73,"context_line":""},{"line_number":74,"context_line":"    def create_workers(self, **kwargs):"},{"line_number":75,"context_line":"        \"\"\""},{"line_number":76,"context_line":"        For each worker spit out a (possibly empty) dict of kwargs to pass"},{"line_number":77,"context_line":"        along to the daemons run method after fork.  The length of elements"}],"source_content_type":"text/x-python","patch_set":5,"id":"1f1a1f67_88550720","line":74,"updated":"2017-07-14 19:50:11.000000000","message":"nit: The method name suggests to me that this method creates the worker processes. Maybe get_worker_args? specify_workers?","commit_id":"fcd24235da12b2bca8dbd550febe68723d593ba1"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"1dc83b5363aa66bc58f8f7c886e21df276f81588","unresolved":false,"context_lines":[{"line_number":71,"context_line":"    specific knowledge about which workers should be doing what."},{"line_number":72,"context_line":"    \"\"\""},{"line_number":73,"context_line":""},{"line_number":74,"context_line":"    def create_workers(self, **kwargs):"},{"line_number":75,"context_line":"        \"\"\""},{"line_number":76,"context_line":"        For each worker spit out a (possibly empty) dict of kwargs to pass"},{"line_number":77,"context_line":"        along to the daemons run method after fork.  The length of elements"}],"source_content_type":"text/x-python","patch_set":5,"id":"1f1a1f67_9d164355","line":74,"in_reply_to":"1f1a1f67_88550720","updated":"2017-07-17 05:15:23.000000000","message":"Done","commit_id":"fcd24235da12b2bca8dbd550febe68723d593ba1"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"737b1413e75c300359d6dd886e7b0e848bef2ba1","unresolved":false,"context_lines":[{"line_number":155,"context_line":"    it will be restarted with the same options, unless the WorkerStrategy is in"},{"line_number":156,"context_line":"    once mode."},{"line_number":157,"context_line":""},{"line_number":158,"context_line":"    :param daemon: an instance of a Daemon (has a run method)"},{"line_number":159,"context_line":"    :param logger: a logger instance"},{"line_number":160,"context_line":"    \"\"\""},{"line_number":161,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"1f1a1f67_3329c437","line":158,"range":{"start_line":158,"start_character":36,"end_line":158,"end_character":61},"updated":"2017-07-14 19:50:11.000000000","message":"WorkerDaemon - and it needs run method plus the other methods (maybe drop the piece in parentheses)","commit_id":"fcd24235da12b2bca8dbd550febe68723d593ba1"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"737b1413e75c300359d6dd886e7b0e848bef2ba1","unresolved":false,"context_lines":[{"line_number":155,"context_line":"    it will be restarted with the same options, unless the WorkerStrategy is in"},{"line_number":156,"context_line":"    once mode."},{"line_number":157,"context_line":""},{"line_number":158,"context_line":"    :param daemon: an instance of a Daemon (has a run method)"},{"line_number":159,"context_line":"    :param logger: a logger instance"},{"line_number":160,"context_line":"    \"\"\""},{"line_number":161,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"1f1a1f67_536440b9","line":158,"range":{"start_line":158,"start_character":5,"end_line":158,"end_character":17},"updated":"2017-07-14 19:50:11.000000000","message":"daemon should prbably be a positional arg for this to make sense","commit_id":"fcd24235da12b2bca8dbd550febe68723d593ba1"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"737b1413e75c300359d6dd886e7b0e848bef2ba1","unresolved":false,"context_lines":[{"line_number":156,"context_line":"    once mode."},{"line_number":157,"context_line":""},{"line_number":158,"context_line":"    :param daemon: an instance of a Daemon (has a run method)"},{"line_number":159,"context_line":"    :param logger: a logger instance"},{"line_number":160,"context_line":"    \"\"\""},{"line_number":161,"context_line":""},{"line_number":162,"context_line":"    def __init__(self, *args, **kwargs):"}],"source_content_type":"text/x-python","patch_set":5,"id":"1f1a1f67_f3e7cc11","line":159,"range":{"start_line":159,"start_character":11,"end_line":159,"end_character":17},"updated":"2017-07-14 19:50:11.000000000","message":"ditto re specify a positional arg - not sure if docstring order implies positional order in method signature","commit_id":"fcd24235da12b2bca8dbd550febe68723d593ba1"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"1dc83b5363aa66bc58f8f7c886e21df276f81588","unresolved":false,"context_lines":[{"line_number":156,"context_line":"    once mode."},{"line_number":157,"context_line":""},{"line_number":158,"context_line":"    :param daemon: an instance of a Daemon (has a run method)"},{"line_number":159,"context_line":"    :param logger: a logger instance"},{"line_number":160,"context_line":"    \"\"\""},{"line_number":161,"context_line":""},{"line_number":162,"context_line":"    def __init__(self, *args, **kwargs):"}],"source_content_type":"text/x-python","patch_set":5,"id":"1f1a1f67_58db197b","line":159,"in_reply_to":"1f1a1f67_f3e7cc11","updated":"2017-07-17 05:15:23.000000000","message":"I don\u0027t really understand what you\u0027re getting at here, but hope it\u0027s N/A with the classes flattened out","commit_id":"fcd24235da12b2bca8dbd550febe68723d593ba1"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"9dfb12aada816b8b69be2c9de4c5d3445a2fa1a9","unresolved":false,"context_lines":[{"line_number":35,"context_line":"    A subclass of Daemon must implement :method:`run_once` and"},{"line_number":36,"context_line":"    :method:`run_forever`."},{"line_number":37,"context_line":""},{"line_number":38,"context_line":"    A subclass of Daemon may override get_worker_args to dispatch arguments to"},{"line_number":39,"context_line":"    individual child process workers and is_healthy to perform context specific"},{"line_number":40,"context_line":"    periodic wellness checks which can reset worker arguments."},{"line_number":41,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"1f1a1f67_1829219a","line":38,"updated":"2017-07-17 06:12:07.000000000","message":"must, may, ick - it\u0027s like this class requires a BS degree in OOP","commit_id":"40ce897e94585cf545e23f320f72cb18f1d1035a"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"e994ec0ec2a8f39afb920127a793d53d83719a94","unresolved":false,"context_lines":[{"line_number":35,"context_line":"    A subclass of Daemon must implement :method:`run_once` and"},{"line_number":36,"context_line":"    :method:`run_forever`."},{"line_number":37,"context_line":""},{"line_number":38,"context_line":"    A subclass of Daemon may override get_worker_args to dispatch arguments to"},{"line_number":39,"context_line":"    individual child process workers and is_healthy to perform context specific"},{"line_number":40,"context_line":"    periodic wellness checks which can reset worker arguments."},{"line_number":41,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"1f1a1f67_4519f11f","line":38,"in_reply_to":"1f1a1f67_1829219a","updated":"2017-07-17 19:51:24.000000000","message":"this is nothing compared to diskfile.py :)","commit_id":"40ce897e94585cf545e23f320f72cb18f1d1035a"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"3a194b43219c6495f4aa98252484163f415e40e2","unresolved":false,"context_lines":[{"line_number":35,"context_line":"    A subclass of Daemon must implement :method:`run_once` and"},{"line_number":36,"context_line":"    :method:`run_forever`."},{"line_number":37,"context_line":""},{"line_number":38,"context_line":"    A subclass of Daemon may override get_worker_args to dispatch arguments to"},{"line_number":39,"context_line":"    individual child process workers and is_healthy to perform context specific"},{"line_number":40,"context_line":"    periodic wellness checks which can reset worker arguments."},{"line_number":41,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"1f1a1f67_3ca15147","line":38,"in_reply_to":"1f1a1f67_4519f11f","updated":"2017-07-18 00:21:13.000000000","message":"that\u0027s my new favorite part about every piece of code I work on \"it\u0027s better than Swift\u0027s diskfile module!\"","commit_id":"40ce897e94585cf545e23f320f72cb18f1d1035a"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"9dfb12aada816b8b69be2c9de4c5d3445a2fa1a9","unresolved":false,"context_lines":[{"line_number":97,"context_line":"    behavior is to invoke the daemon\u0027s run method from within the parent"},{"line_number":98,"context_line":"    process.  When the run method returns the parent process will exit."},{"line_number":99,"context_line":""},{"line_number":100,"context_line":"    However, if the Daemon returns a non-empty iterable from get_worker_args,"},{"line_number":101,"context_line":"    the daemon\u0027s run method will be invoked in child process, with the"},{"line_number":102,"context_line":"    arguments provided from the parent process\u0027s instance of the deamon.  If a"},{"line_number":103,"context_line":"    child process exits it will be restarted with the same options, unless the"}],"source_content_type":"text/x-python","patch_set":7,"id":"1f1a1f67_f81b057e","line":100,"updated":"2017-07-17 06:12:07.000000000","message":"so basically what you\u0027re saying is... this class does two things :\\","commit_id":"40ce897e94585cf545e23f320f72cb18f1d1035a"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"9dfb12aada816b8b69be2c9de4c5d3445a2fa1a9","unresolved":false,"context_lines":[{"line_number":111,"context_line":"        self.daemon \u003d daemon"},{"line_number":112,"context_line":"        self.logger \u003d logger"},{"line_number":113,"context_line":"        self.running \u003d False"},{"line_number":114,"context_line":"        # only used by multi-worker strategy"},{"line_number":115,"context_line":"        self.options_by_pid \u003d {}"},{"line_number":116,"context_line":"        self.unspawned_worker_options \u003d []"},{"line_number":117,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"1f1a1f67_98165155","line":114,"updated":"2017-07-17 06:12:07.000000000","message":"and at any given time only half of it\u0027s state should be loaded in my head if i\u0027m trying to understand it... in fact... that\u0027s the common case :\\","commit_id":"40ce897e94585cf545e23f320f72cb18f1d1035a"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"9dfb12aada816b8b69be2c9de4c5d3445a2fa1a9","unresolved":false,"context_lines":[{"line_number":132,"context_line":""},{"line_number":133,"context_line":"    def _run_inline(self, once\u003dFalse, **kwargs):"},{"line_number":134,"context_line":"        \"\"\"Run the daemon\"\"\""},{"line_number":135,"context_line":"        self.daemon.run(once\u003donce, **kwargs)"},{"line_number":136,"context_line":""},{"line_number":137,"context_line":"    def run(self, once\u003dFalse, **kwargs):"},{"line_number":138,"context_line":"        \"\"\"Daemonize and execute our strategy\"\"\""}],"source_content_type":"text/x-python","patch_set":7,"id":"1f1a1f67_780ff536","line":135,"updated":"2017-07-17 06:12:07.000000000","message":"run, _run, _run_inline - gd","commit_id":"40ce897e94585cf545e23f320f72cb18f1d1035a"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"c9fba230dfe4e041e6b434657c252159c14f2318","unresolved":false,"context_lines":[{"line_number":210,"context_line":"        if not self.unspawned_worker_options:"},{"line_number":211,"context_line":"            return self._run_inline(once, **kwargs)"},{"line_number":212,"context_line":"        for per_worker_options in self.iter_unspawned_workers():"},{"line_number":213,"context_line":"            if self._fork(once, **per_worker_options) \u003d\u003d 0:"},{"line_number":214,"context_line":"                return 0"},{"line_number":215,"context_line":"        while self.running:"},{"line_number":216,"context_line":"            if self.abort_workers_if_daemon_would_like():"}],"source_content_type":"text/x-python","patch_set":8,"id":"1f1a1f67_3d7fe5ab","line":213,"range":{"start_line":213,"start_character":15,"end_line":213,"end_character":58},"updated":"2017-07-19 00:38:01.000000000","message":"This will never be true, yeah?","commit_id":"52d92732cd19f73893f04738a89d8d9eaa2df4fd"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"c9fba230dfe4e041e6b434657c252159c14f2318","unresolved":false,"context_lines":[{"line_number":218,"context_line":"            self.check_on_all_running_workers()"},{"line_number":219,"context_line":"            if not once:"},{"line_number":220,"context_line":"                for per_worker_options in self.iter_unspawned_workers():"},{"line_number":221,"context_line":"                    if self._fork(once, **per_worker_options) \u003d\u003d 0:"},{"line_number":222,"context_line":"                        return 0"},{"line_number":223,"context_line":"            else:"},{"line_number":224,"context_line":"                if not self.spawned_pids():"}],"source_content_type":"text/x-python","patch_set":8,"id":"1f1a1f67_fd1ccd78","line":221,"range":{"start_line":221,"start_character":23,"end_line":221,"end_character":66},"updated":"2017-07-19 00:38:01.000000000","message":"ditto","commit_id":"52d92732cd19f73893f04738a89d8d9eaa2df4fd"}],"swift/obj/reconstructor.py":[{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"1ff882b8599733c9d7cb831fe0ddc29fec812c6f","unresolved":false,"context_lines":[{"line_number":94,"context_line":"        }"},{"line_number":95,"context_line":""},{"line_number":96,"context_line":""},{"line_number":97,"context_line":"def parse_override_options(**kwargs):"},{"line_number":98,"context_line":"    override_devices \u003d kwargs.get(\u0027override_devices\u0027) or list_from_csv("},{"line_number":99,"context_line":"        kwargs.get(\u0027devices\u0027))"},{"line_number":100,"context_line":"    override_partitions \u003d kwargs.get(\u0027override_partitions\u0027) or ["}],"source_content_type":"text/x-python","patch_set":1,"id":"3f1d235d_147edb6a","line":97,"updated":"2017-06-30 14:37:15.000000000","message":"+1","commit_id":"18a394fff8aac3a40e4b70a9629cedf0db474d55"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"1ff882b8599733c9d7cb831fe0ddc29fec812c6f","unresolved":false,"context_lines":[{"line_number":170,"context_line":"        # N.B. to maintain compatibility with legacy configs if name this"},{"line_number":171,"context_line":"        # option workers because the object-server uses that option name in the"},{"line_number":172,"context_line":"        # DEFAULT section"},{"line_number":173,"context_line":"        self.reconstructor_workers \u003d int(conf.get(\u0027reconstructor_workers\u0027, 0))"},{"line_number":174,"context_line":"        self.per_disk_stats \u003d config_true_value(conf.get("},{"line_number":175,"context_line":"            \u0027per_disk_stats\u0027, True if self.workers \u003e 0 else False))"},{"line_number":176,"context_line":"        self.policies \u003d [policy for policy in POLICIES"}],"source_content_type":"text/x-python","patch_set":1,"id":"3f1d235d_f4f7dff4","line":173,"range":{"start_line":173,"start_character":51,"end_line":173,"end_character":72},"updated":"2017-06-30 14:37:15.000000000","message":"nit: max_reconstructor_workers ?","commit_id":"18a394fff8aac3a40e4b70a9629cedf0db474d55"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"1ff882b8599733c9d7cb831fe0ddc29fec812c6f","unresolved":false,"context_lines":[{"line_number":972,"context_line":"        # return a list of jobs for this part"},{"line_number":973,"context_line":"        return jobs"},{"line_number":974,"context_line":""},{"line_number":975,"context_line":"    def get_policy2devices(self):"},{"line_number":976,"context_line":"        ips \u003d whataremyips(self.bind_ip)"},{"line_number":977,"context_line":"        policy2devices \u003d {}"},{"line_number":978,"context_line":"        for policy in self.policies:"}],"source_content_type":"text/x-python","patch_set":1,"id":"3f1d235d_d44aa325","line":975,"updated":"2017-06-30 14:37:15.000000000","message":"+1","commit_id":"18a394fff8aac3a40e4b70a9629cedf0db474d55"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"861dccd4c572ed6ebb3aa936a305b12aca24cf19","unresolved":false,"context_lines":[{"line_number":170,"context_line":"        # N.B. to maintain compatibility with legacy configs if name this"},{"line_number":171,"context_line":"        # option workers because the object-server uses that option name in the"},{"line_number":172,"context_line":"        # DEFAULT section"},{"line_number":173,"context_line":"        self.reconstructor_workers \u003d int(conf.get(\u0027reconstructor_workers\u0027, 0))"},{"line_number":174,"context_line":"        self.per_disk_stats \u003d config_true_value(conf.get("},{"line_number":175,"context_line":"            \u0027per_disk_stats\u0027,"},{"line_number":176,"context_line":"            True if self.reconstructor_workers \u003e 0 else False))"}],"source_content_type":"text/x-python","patch_set":2,"id":"3f1d235d_ba773709","line":173,"updated":"2017-07-08 01:27:09.000000000","message":"I keep adding some silly validation for reconstructor_workers \u003d -1 but there\u0027s not that much prior art and mostly it just reminds me we should be using oslo.config","commit_id":"9f3e7cf1ed98ad65d57295ebe51260280054aa72"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"d16bf21b5758782ff39d49f681679a02be7f01d1","unresolved":false,"context_lines":[{"line_number":170,"context_line":"        # N.B. to maintain compatibility with legacy configs if name this"},{"line_number":171,"context_line":"        # option workers because the object-server uses that option name in the"},{"line_number":172,"context_line":"        # DEFAULT section"},{"line_number":173,"context_line":"        self.reconstructor_workers \u003d int(conf.get(\u0027reconstructor_workers\u0027, 0))"},{"line_number":174,"context_line":"        self.per_disk_stats \u003d config_true_value(conf.get("},{"line_number":175,"context_line":"            \u0027per_disk_stats\u0027,"},{"line_number":176,"context_line":"            True if self.reconstructor_workers \u003e 0 else False))"}],"source_content_type":"text/x-python","patch_set":2,"id":"3f1d235d_d1f8757d","line":173,"in_reply_to":"3f1d235d_ba773709","updated":"2017-07-10 18:40:25.000000000","message":"I wanted to point you to [1] but it doesn\u0027t cover 0 :/\n\n[1] https://github.com/openstack/swift/blob/e94b383655bafcd9df72c2f02b8e4864b0c3cd85/swift/common/utils.py#L350-L350","commit_id":"9f3e7cf1ed98ad65d57295ebe51260280054aa72"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"861dccd4c572ed6ebb3aa936a305b12aca24cf19","unresolved":false,"context_lines":[{"line_number":218,"context_line":""},{"line_number":219,"context_line":"    @property"},{"line_number":220,"context_line":"    def workers(self):"},{"line_number":221,"context_line":"        return self.reconstructor_workers"},{"line_number":222,"context_line":""},{"line_number":223,"context_line":"    def create_workers(self, **kwargs):"},{"line_number":224,"context_line":"        \"\"\""}],"source_content_type":"text/x-python","patch_set":2,"id":"3f1d235d_fa1fefb4","line":221,"updated":"2017-07-08 01:27:09.000000000","message":"I could also make the interface `should_use_workers()` and have this\n\n    return self.reconstructor_workers \u003e 0","commit_id":"9f3e7cf1ed98ad65d57295ebe51260280054aa72"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"861dccd4c572ed6ebb3aa936a305b12aca24cf19","unresolved":false,"context_lines":[{"line_number":227,"context_line":"        to be spawned according to the configured worker count."},{"line_number":228,"context_line":""},{"line_number":229,"context_line":"        :param kwargs: optional overrides from the command line"},{"line_number":230,"context_line":"        \"\"\""},{"line_number":231,"context_line":"        override_options \u003d parse_override_options(**kwargs)"},{"line_number":232,"context_line":"        all_devices \u003d self.all_local_devices()"},{"line_number":233,"context_line":"        max_devices_per_worker \u003d int(math.ceil("}],"source_content_type":"text/x-python","patch_set":2,"id":"3f1d235d_5a1143ac","line":230,"updated":"2017-07-08 01:27:09.000000000","message":"instead of the property above, we could make the interface require us to return no workers options when we want to fallback to inline execution.\n\n        if not self.reconstructor_workers:\n            return\n\nI think it looks weird because I ended up doing this as generator instead of returning .append and return worker_options","commit_id":"9f3e7cf1ed98ad65d57295ebe51260280054aa72"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"d16bf21b5758782ff39d49f681679a02be7f01d1","unresolved":false,"context_lines":[{"line_number":227,"context_line":"        to be spawned according to the configured worker count."},{"line_number":228,"context_line":""},{"line_number":229,"context_line":"        :param kwargs: optional overrides from the command line"},{"line_number":230,"context_line":"        \"\"\""},{"line_number":231,"context_line":"        override_options \u003d parse_override_options(**kwargs)"},{"line_number":232,"context_line":"        all_devices \u003d self.all_local_devices()"},{"line_number":233,"context_line":"        max_devices_per_worker \u003d int(math.ceil("}],"source_content_type":"text/x-python","patch_set":2,"id":"3f1d235d_f13ed1d8","line":230,"in_reply_to":"3f1d235d_5a1143ac","updated":"2017-07-10 18:40:25.000000000","message":"we might want this *anyway* so that if this method gets called when self.workers (or self.reconstructor_workers) is zero we don\u0027t get a divide by zero error - I know, it is not currently called in that situation, but could arise if the strategy implementation ever changed","commit_id":"9f3e7cf1ed98ad65d57295ebe51260280054aa72"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"861dccd4c572ed6ebb3aa936a305b12aca24cf19","unresolved":false,"context_lines":[{"line_number":254,"context_line":"            self.aggregate_recon_update()"},{"line_number":255,"context_line":"        return all(self.check_ring(p.object_ring) for p in self.policies)"},{"line_number":256,"context_line":""},{"line_number":257,"context_line":"    def aggregate_recon_update(self):"},{"line_number":258,"context_line":"        with open(self.rcache) as f:"},{"line_number":259,"context_line":"            existing_data \u003d json.load(f)"},{"line_number":260,"context_line":"        first_start \u003d time.time()"}],"source_content_type":"text/x-python","patch_set":2,"id":"3f1d235d_9aca3b18","line":257,"updated":"2017-07-08 01:27:09.000000000","message":"this mess of hacks could *really* use a doc string\n\nI have some sketch on some note pad from when I was trying to draw up what I intended - the goal is that each time a worker drops stats for a disk(s) the aggregates shift forward their \"_last\" and the \"_time\" is a pseudo-duration that\u0027s equal to the total runtime since earliest reported \"start\" and the most recently reported \"finish\".\n\n... if that makes sense\n\nI hope it\u0027s a sufficiently accurate representation of a aggregate node wide \"cycle\" as we still use cycles as a heuristic signal into ring rebalancing.","commit_id":"9f3e7cf1ed98ad65d57295ebe51260280054aa72"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"861dccd4c572ed6ebb3aa936a305b12aca24cf19","unresolved":false,"context_lines":[{"line_number":279,"context_line":"            \u0027object_reconstruction_per_disk\u0027, {}).keys())"},{"line_number":280,"context_line":"        clear_update \u003d {d: {} for d in found_devices ^ all_local_devices}"},{"line_number":281,"context_line":"        if clear_update:"},{"line_number":282,"context_line":"            recon_update[\u0027object_reconstruction_per_disk\u0027] \u003d clear_update"},{"line_number":283,"context_line":"        dump_recon_cache(recon_update, self.rcache, self.logger)"},{"line_number":284,"context_line":""},{"line_number":285,"context_line":"    def load_object_ring(self, policy):"}],"source_content_type":"text/x-python","patch_set":2,"id":"3f1d235d_9af35b74","line":282,"updated":"2017-07-08 01:27:09.000000000","message":"this takes advantage of some behaviors in the recon interface to preserve pre-existing keys in the recon dump, and only update keys that are in the update, and also remove keys when the value is an empty dict.\n\nI functinally tested this and it *seems* right, but I\u0027ll probably have to write tests that go all the way to disk to confirm for myself i\u0027m doing this right.","commit_id":"9f3e7cf1ed98ad65d57295ebe51260280054aa72"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"61c1fee75470bd8943f1d0f30b1f7b257478ee94","unresolved":false,"context_lines":[{"line_number":1091,"context_line":"        self.reconstruct("},{"line_number":1092,"context_line":"            override_devices\u003doverride_devices,"},{"line_number":1093,"context_line":"            override_partitions\u003doverride_partitions)"},{"line_number":1094,"context_line":"        total \u003d (time.time() - start) / 60"},{"line_number":1095,"context_line":"        self.logger.info("},{"line_number":1096,"context_line":"            _(\"Object reconstruction complete (once). (%.02f minutes)\"), total)"},{"line_number":1097,"context_line":"        if not (override_partitions or override_devices):"}],"source_content_type":"text/x-python","patch_set":3,"id":"3f1d235d_5bbe9065","side":"PARENT","line":1094,"updated":"2017-07-11 22:18:04.000000000","message":"oops!  I forgot the / 60 in the aggregation!","commit_id":"8472cb653828a2f8026c9b31e2d447baab9ddd84"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"697d4fa4d527feadea01697ab5303e7174bb36a2","unresolved":false,"context_lines":[{"line_number":1113,"context_line":"            self.logger.info(_(\"Starting object reconstruction pass.\"))"},{"line_number":1114,"context_line":"            # Run the reconstructor"},{"line_number":1115,"context_line":"            self.reconstruct()"},{"line_number":1116,"context_line":"            total \u003d (time.time() - start) / 60"},{"line_number":1117,"context_line":"            self.logger.info("},{"line_number":1118,"context_line":"                _(\"Object reconstruction complete. (%.02f minutes)\"), total)"},{"line_number":1119,"context_line":"            dump_recon_cache({\u0027object_reconstruction_time\u0027: total,"}],"source_content_type":"text/x-python","patch_set":4,"id":"3f1d235d_311d3373","side":"PARENT","line":1116,"updated":"2017-07-12 00:11:57.000000000","message":"all about the minutes!","commit_id":"5cb1303d5baed0a5beab7cf6de8e2ec88f8ab49f"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"697d4fa4d527feadea01697ab5303e7174bb36a2","unresolved":false,"context_lines":[{"line_number":171,"context_line":"        # option can not be workers because the object-server uses that option"},{"line_number":172,"context_line":"        # name in the DEFAULT section"},{"line_number":173,"context_line":"        self.reconstructor_workers \u003d int(conf.get(\u0027reconstructor_workers\u0027, 0))"},{"line_number":174,"context_line":"        self.per_disk_stats \u003d True if self.reconstructor_workers \u003e 0 else False"},{"line_number":175,"context_line":"        self.policies \u003d [policy for policy in POLICIES"},{"line_number":176,"context_line":"                         if policy.policy_type \u003d\u003d EC_POLICY]"},{"line_number":177,"context_line":"        self.stats_interval \u003d int(conf.get(\u0027stats_interval\u0027, \u0027300\u0027))"}],"source_content_type":"text/x-python","patch_set":4,"id":"3f1d235d_71296b99","line":174,"updated":"2017-07-12 00:11:57.000000000","message":"I can\u0027t really figure out how/why this would be independently configurable","commit_id":"5f6664dd83a1ce841125f98f8f666b46cf9b13ef"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"737b1413e75c300359d6dd886e7b0e848bef2ba1","unresolved":false,"context_lines":[{"line_number":227,"context_line":"        override_options \u003d parse_override_options(**kwargs)"},{"line_number":228,"context_line":"        all_devices \u003d self.all_local_devices()"},{"line_number":229,"context_line":"        if not all_devices:"},{"line_number":230,"context_line":"            # we only need a single worker to do nothing until a ring change"},{"line_number":231,"context_line":"            yield override_options"},{"line_number":232,"context_line":"            return"},{"line_number":233,"context_line":"        max_devices_per_worker \u003d int(math.ceil("}],"source_content_type":"text/x-python","patch_set":5,"id":"1f1a1f67_88ad028a","line":230,"updated":"2017-07-14 19:50:11.000000000","message":"ok. so this avoids the WorkersStrategy reverting to inline mode when we want is to go multi-process as soon as we have devices.","commit_id":"fcd24235da12b2bca8dbd550febe68723d593ba1"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"1dc83b5363aa66bc58f8f7c886e21df276f81588","unresolved":false,"context_lines":[{"line_number":227,"context_line":"        override_options \u003d parse_override_options(**kwargs)"},{"line_number":228,"context_line":"        all_devices \u003d self.all_local_devices()"},{"line_number":229,"context_line":"        if not all_devices:"},{"line_number":230,"context_line":"            # we only need a single worker to do nothing until a ring change"},{"line_number":231,"context_line":"            yield override_options"},{"line_number":232,"context_line":"            return"},{"line_number":233,"context_line":"        max_devices_per_worker \u003d int(math.ceil("}],"source_content_type":"text/x-python","patch_set":5,"id":"1f1a1f67_18fda1b0","line":230,"in_reply_to":"1f1a1f67_88ad028a","updated":"2017-07-17 05:15:23.000000000","message":"Done","commit_id":"fcd24235da12b2bca8dbd550febe68723d593ba1"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"737b1413e75c300359d6dd886e7b0e848bef2ba1","unresolved":false,"context_lines":[{"line_number":1206,"context_line":"            \u0027object_reconstruction_time\u0027: total,"},{"line_number":1207,"context_line":"            \u0027object_reconstruction_last\u0027: time.time(),"},{"line_number":1208,"context_line":"        }"},{"line_number":1209,"context_line":"        if self.per_disk_stats:"},{"line_number":1210,"context_line":"            devices \u003d override_devices or self.all_local_devices()"},{"line_number":1211,"context_line":"            recon_update[\u0027pid\u0027] \u003d os.getpid()"},{"line_number":1212,"context_line":"            recon_update \u003d {\u0027object_reconstruction_per_disk\u0027: {"}],"source_content_type":"text/x-python","patch_set":5,"id":"1f1a1f67_a8cb1e70","line":1209,"range":{"start_line":1209,"start_character":16,"end_line":1209,"end_character":30},"updated":"2017-07-14 19:50:11.000000000","message":"nit: could be just \n\n  if self.reconstructor_workers\n\nelsewhere \u0027per_disk_stats\u0027 is used as name of the stats dict so it could be a little confusing to also have the instance var being a boolean.","commit_id":"fcd24235da12b2bca8dbd550febe68723d593ba1"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"1dc83b5363aa66bc58f8f7c886e21df276f81588","unresolved":false,"context_lines":[{"line_number":1206,"context_line":"            \u0027object_reconstruction_time\u0027: total,"},{"line_number":1207,"context_line":"            \u0027object_reconstruction_last\u0027: time.time(),"},{"line_number":1208,"context_line":"        }"},{"line_number":1209,"context_line":"        if self.per_disk_stats:"},{"line_number":1210,"context_line":"            devices \u003d override_devices or self.all_local_devices()"},{"line_number":1211,"context_line":"            recon_update[\u0027pid\u0027] \u003d os.getpid()"},{"line_number":1212,"context_line":"            recon_update \u003d {\u0027object_reconstruction_per_disk\u0027: {"}],"source_content_type":"text/x-python","patch_set":5,"id":"1f1a1f67_b8258d5e","line":1209,"in_reply_to":"1f1a1f67_a8cb1e70","updated":"2017-07-17 05:15:23.000000000","message":"Done","commit_id":"fcd24235da12b2bca8dbd550febe68723d593ba1"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"737b1413e75c300359d6dd886e7b0e848bef2ba1","unresolved":false,"context_lines":[{"line_number":1211,"context_line":"            recon_update[\u0027pid\u0027] \u003d os.getpid()"},{"line_number":1212,"context_line":"            recon_update \u003d {\u0027object_reconstruction_per_disk\u0027: {"},{"line_number":1213,"context_line":"                d: recon_update for d in devices}}"},{"line_number":1214,"context_line":"        dump_recon_cache(recon_update, self.rcache, self.logger)"},{"line_number":1215,"context_line":""},{"line_number":1216,"context_line":"    def run_once(self, *args, **kwargs):"},{"line_number":1217,"context_line":"        start \u003d time.time()"}],"source_content_type":"text/x-python","patch_set":5,"id":"1f1a1f67_5c38c89e","line":1214,"updated":"2017-07-14 19:50:11.000000000","message":"I think it is possible to make the per disk stats only be dumped when we have \u003e1 worker if that is desirable:\n\nhttps://gist.github.com/alistairncoles/c7445bdc3bdbd48068cfba7256725156\n\n(since discussed - meh! although the gist does clean up object_reconstruction_per_disk if not in workers mode","commit_id":"fcd24235da12b2bca8dbd550febe68723d593ba1"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"1dc83b5363aa66bc58f8f7c886e21df276f81588","unresolved":false,"context_lines":[{"line_number":1211,"context_line":"            recon_update[\u0027pid\u0027] \u003d os.getpid()"},{"line_number":1212,"context_line":"            recon_update \u003d {\u0027object_reconstruction_per_disk\u0027: {"},{"line_number":1213,"context_line":"                d: recon_update for d in devices}}"},{"line_number":1214,"context_line":"        dump_recon_cache(recon_update, self.rcache, self.logger)"},{"line_number":1215,"context_line":""},{"line_number":1216,"context_line":"    def run_once(self, *args, **kwargs):"},{"line_number":1217,"context_line":"        start \u003d time.time()"}],"source_content_type":"text/x-python","patch_set":5,"id":"1f1a1f67_5820b948","line":1214,"in_reply_to":"1f1a1f67_5c38c89e","updated":"2017-07-17 05:15:23.000000000","message":"Done","commit_id":"fcd24235da12b2bca8dbd550febe68723d593ba1"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"e994ec0ec2a8f39afb920127a793d53d83719a94","unresolved":false,"context_lines":[{"line_number":289,"context_line":"            recon_update \u003d {}"},{"line_number":290,"context_line":"        found_devices \u003d set(existing_data.get("},{"line_number":291,"context_line":"            \u0027object_reconstruction_per_disk\u0027, {}).keys())"},{"line_number":292,"context_line":"        clear_update \u003d {d: {} for d in found_devices ^ all_local_devices}"},{"line_number":293,"context_line":"        if clear_update:"},{"line_number":294,"context_line":"            recon_update[\u0027object_reconstruction_per_disk\u0027] \u003d clear_update"},{"line_number":295,"context_line":"        dump_recon_cache(recon_update, self.rcache, self.logger)"}],"source_content_type":"text/x-python","patch_set":7,"id":"1f1a1f67_7ea7cea8","line":292,"updated":"2017-07-17 19:51:24.000000000","message":"for a device that is a local device but not yet reported, there is a read-modify-write race here:\n\n  this function          worker\n  -------------          ------\n\n   reads stats\n   aggregates\n                         write missing\n                           per disk stats\n   clears missing\n     per disk stats\n\n\nit would only happen when per disk stats are missing i.e. at start up or after a ring change, but I don\u0027t think we need to clear devices that *are* local devices anyway, so:\n\n  clear_update \u003d {d: {} for d in found_devices if d not in all_local_devices}\n\nsee https://review.openstack.org/#/c/484448/1 for fix and test plus some other test fixup","commit_id":"40ce897e94585cf545e23f320f72cb18f1d1035a"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"3a194b43219c6495f4aa98252484163f415e40e2","unresolved":false,"context_lines":[{"line_number":289,"context_line":"            recon_update \u003d {}"},{"line_number":290,"context_line":"        found_devices \u003d set(existing_data.get("},{"line_number":291,"context_line":"            \u0027object_reconstruction_per_disk\u0027, {}).keys())"},{"line_number":292,"context_line":"        clear_update \u003d {d: {} for d in found_devices ^ all_local_devices}"},{"line_number":293,"context_line":"        if clear_update:"},{"line_number":294,"context_line":"            recon_update[\u0027object_reconstruction_per_disk\u0027] \u003d clear_update"},{"line_number":295,"context_line":"        dump_recon_cache(recon_update, self.rcache, self.logger)"}],"source_content_type":"text/x-python","patch_set":7,"id":"1f1a1f67_2aad8b9a","line":292,"in_reply_to":"1f1a1f67_7ea7cea8","updated":"2017-07-18 00:21:13.000000000","message":"+2 this should definitely get fixed, this is a bad behavior","commit_id":"40ce897e94585cf545e23f320f72cb18f1d1035a"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"c9fba230dfe4e041e6b434657c252159c14f2318","unresolved":false,"context_lines":[{"line_number":227,"context_line":"        all_devices \u003d self.all_local_devices()"},{"line_number":228,"context_line":"        if not all_devices:"},{"line_number":229,"context_line":"            # we only need a single worker to do nothing until a ring change"},{"line_number":230,"context_line":"            yield override_options"},{"line_number":231,"context_line":"            return"},{"line_number":232,"context_line":"        max_devices_per_worker \u003d int(math.ceil("},{"line_number":233,"context_line":"            1.0 * len(all_devices) / self.reconstructor_workers))"}],"source_content_type":"text/x-python","patch_set":8,"id":"1f1a1f67_8076926f","line":230,"updated":"2017-07-19 00:38:01.000000000","message":"Do we even need a worker? Should we just return, no yield?","commit_id":"52d92732cd19f73893f04738a89d8d9eaa2df4fd"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"a9c09b33d0d04dee00919fa16a88ef3fa837078c","unresolved":false,"context_lines":[{"line_number":227,"context_line":"        all_devices \u003d self.all_local_devices()"},{"line_number":228,"context_line":"        if not all_devices:"},{"line_number":229,"context_line":"            # we only need a single worker to do nothing until a ring change"},{"line_number":230,"context_line":"            yield override_options"},{"line_number":231,"context_line":"            return"},{"line_number":232,"context_line":"        max_devices_per_worker \u003d int(math.ceil("},{"line_number":233,"context_line":"            1.0 * len(all_devices) / self.reconstructor_workers))"}],"source_content_type":"text/x-python","patch_set":8,"id":"1f1a1f67_2b8764b9","line":230,"in_reply_to":"1f1a1f67_8076926f","updated":"2017-07-20 13:41:50.000000000","message":"The idea here is to prevent the strategy falling back to \u0027inline\u0027 mode if no worker args are returned because there were no devices *at the moment*. We want to the strategy to be in workers mode so that the reconstructor gets calls to is_healthy and can change the worker args if/when ring(s) change.","commit_id":"52d92732cd19f73893f04738a89d8d9eaa2df4fd"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"c9fba230dfe4e041e6b434657c252159c14f2318","unresolved":false,"context_lines":[{"line_number":233,"context_line":"            1.0 * len(all_devices) / self.reconstructor_workers))"},{"line_number":234,"context_line":"        n \u003d int(math.ceil(1.0 * len(all_devices) / max_devices_per_worker))"},{"line_number":235,"context_line":"        if override_options[\u0027override_devices\u0027]:"},{"line_number":236,"context_line":"            all_devices \u0026\u003d override_options[\u0027override_devices\u0027]"},{"line_number":237,"context_line":"        seq \u003d list(all_devices)"},{"line_number":238,"context_line":"        override_devices_per_worker \u003d [seq[i::n] for i in range(n)]"},{"line_number":239,"context_line":"        for override_devices in override_devices_per_worker:"}],"source_content_type":"text/x-python","patch_set":8,"id":"1f1a1f67_e019c6d6","line":236,"updated":"2017-07-19 00:38:01.000000000","message":"Shouldn\u0027t we do this *before* calculating n?\n\nWhy bother calculating n, anyway? Why not just use\n\n n \u003d min(len(all_devices), self.reconstructor_workers)\n\nbelow? I\u0027m not sure I get the benefit of, e.g., having 3x3 instead of 1x3+3x2 when distributing 9 disks to (up to) 4 workers.","commit_id":"52d92732cd19f73893f04738a89d8d9eaa2df4fd"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"a9c09b33d0d04dee00919fa16a88ef3fa837078c","unresolved":false,"context_lines":[{"line_number":233,"context_line":"            1.0 * len(all_devices) / self.reconstructor_workers))"},{"line_number":234,"context_line":"        n \u003d int(math.ceil(1.0 * len(all_devices) / max_devices_per_worker))"},{"line_number":235,"context_line":"        if override_options[\u0027override_devices\u0027]:"},{"line_number":236,"context_line":"            all_devices \u0026\u003d override_options[\u0027override_devices\u0027]"},{"line_number":237,"context_line":"        seq \u003d list(all_devices)"},{"line_number":238,"context_line":"        override_devices_per_worker \u003d [seq[i::n] for i in range(n)]"},{"line_number":239,"context_line":"        for override_devices in override_devices_per_worker:"}],"source_content_type":"text/x-python","patch_set":8,"id":"1f1a1f67_ab500f0c","line":236,"range":{"start_line":236,"start_character":24,"end_line":236,"end_character":26},"updated":"2017-07-20 13:41:50.000000000","message":"all_devices is a set but override_options[\u0027override_devices\u0027] is a list, cannot use \u0026\u003d with those types\n\nI think maybe something like:\nhttps://gist.github.com/alistairncoles/5ebd837ebe86d5805fee116b8ea7f822","commit_id":"52d92732cd19f73893f04738a89d8d9eaa2df4fd"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"0eda1fc3cb9c82f11f4af5d15be4945d5dc73f8f","unresolved":false,"context_lines":[{"line_number":233,"context_line":"            1.0 * len(all_devices) / self.reconstructor_workers))"},{"line_number":234,"context_line":"        n \u003d int(math.ceil(1.0 * len(all_devices) / max_devices_per_worker))"},{"line_number":235,"context_line":"        if override_options[\u0027override_devices\u0027]:"},{"line_number":236,"context_line":"            all_devices \u0026\u003d override_options[\u0027override_devices\u0027]"},{"line_number":237,"context_line":"        seq \u003d list(all_devices)"},{"line_number":238,"context_line":"        override_devices_per_worker \u003d [seq[i::n] for i in range(n)]"},{"line_number":239,"context_line":"        for override_devices in override_devices_per_worker:"}],"source_content_type":"text/x-python","patch_set":8,"id":"1f1a1f67_7ded108a","line":236,"in_reply_to":"1f1a1f67_26879bb9","updated":"2017-07-20 18:37:47.000000000","message":"There are some cases that produce possibly suboptimal distributions e.g. 8 workers, 9 devices will use 5 workers with (2,2,2,2,1) but 8 workers with (1,1,1,1,1,1,1,2) has same \u0027non-uniformity\u0027 i.e. delta of 1 in number of devices/worker but would be faster.\n\nIMO we can work on improving the algorithm as a follow up. The change as it stands is an improvement so I would be happy for it to merge with the current algorithm (obviously with the bug fixes) and iterate from there.","commit_id":"52d92732cd19f73893f04738a89d8d9eaa2df4fd"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"e17013a69ef2bb33b061dccc3c4e5fa4e890392c","unresolved":false,"context_lines":[{"line_number":233,"context_line":"            1.0 * len(all_devices) / self.reconstructor_workers))"},{"line_number":234,"context_line":"        n \u003d int(math.ceil(1.0 * len(all_devices) / max_devices_per_worker))"},{"line_number":235,"context_line":"        if override_options[\u0027override_devices\u0027]:"},{"line_number":236,"context_line":"            all_devices \u0026\u003d override_options[\u0027override_devices\u0027]"},{"line_number":237,"context_line":"        seq \u003d list(all_devices)"},{"line_number":238,"context_line":"        override_devices_per_worker \u003d [seq[i::n] for i in range(n)]"},{"line_number":239,"context_line":"        for override_devices in override_devices_per_worker:"}],"source_content_type":"text/x-python","patch_set":8,"id":"ff346bd7_db0e7f31","line":236,"in_reply_to":"1f1a1f67_26879bb9","updated":"2017-07-21 19:10:19.000000000","message":"Update: apparently the intent is to have similar number of devs per worker whether running a subset of override devices or all devices.\n\nSo max_devices_per_worker should use all_devices. However, n should be calculated using actual number of devices. See dependent fixup patch https://review.openstack.org/486167","commit_id":"52d92732cd19f73893f04738a89d8d9eaa2df4fd"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"a9c09b33d0d04dee00919fa16a88ef3fa837078c","unresolved":false,"context_lines":[{"line_number":233,"context_line":"            1.0 * len(all_devices) / self.reconstructor_workers))"},{"line_number":234,"context_line":"        n \u003d int(math.ceil(1.0 * len(all_devices) / max_devices_per_worker))"},{"line_number":235,"context_line":"        if override_options[\u0027override_devices\u0027]:"},{"line_number":236,"context_line":"            all_devices \u0026\u003d override_options[\u0027override_devices\u0027]"},{"line_number":237,"context_line":"        seq \u003d list(all_devices)"},{"line_number":238,"context_line":"        override_devices_per_worker \u003d [seq[i::n] for i in range(n)]"},{"line_number":239,"context_line":"        for override_devices in override_devices_per_worker:"}],"source_content_type":"text/x-python","patch_set":8,"id":"1f1a1f67_26879bb9","line":236,"in_reply_to":"1f1a1f67_e019c6d6","updated":"2017-07-20 13:41:50.000000000","message":"1. Yes - this is in wrong order\n\n2. \nThe goal, as Clay explained to me, is to prefer uniformity of load over maximal use of workers - so 3x3 is preferable to 1x3+3x2 in your example. One reason being an attempt to have worker report stats somewhat uniformly.","commit_id":"52d92732cd19f73893f04738a89d8d9eaa2df4fd"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"0eda1fc3cb9c82f11f4af5d15be4945d5dc73f8f","unresolved":false,"context_lines":[{"line_number":234,"context_line":"        n \u003d int(math.ceil(1.0 * len(all_devices) / max_devices_per_worker))"},{"line_number":235,"context_line":"        if override_options[\u0027override_devices\u0027]:"},{"line_number":236,"context_line":"            all_devices \u0026\u003d override_options[\u0027override_devices\u0027]"},{"line_number":237,"context_line":"        seq \u003d list(all_devices)"},{"line_number":238,"context_line":"        override_devices_per_worker \u003d [seq[i::n] for i in range(n)]"},{"line_number":239,"context_line":"        for override_devices in override_devices_per_worker:"},{"line_number":240,"context_line":"            options \u003d {}"}],"source_content_type":"text/x-python","patch_set":8,"id":"1f1a1f67_84d41dbf","line":237,"range":{"start_line":237,"start_character":19,"end_line":237,"end_character":30},"updated":"2017-07-20 18:37:47.000000000","message":"if override_devices were given but none are in all_local_devices then all_devices is an empty list, but we\u0027ll still yield n workers. I think the filtering of devices actually needs to move all the way ahead of line 228.\n\nthis gist improves on my earlier one\n\nhttps://gist.github.com/alistairncoles/89abde313ed1c61a469de00563e4c43b","commit_id":"52d92732cd19f73893f04738a89d8d9eaa2df4fd"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"c9fba230dfe4e041e6b434657c252159c14f2318","unresolved":false,"context_lines":[{"line_number":239,"context_line":"        for override_devices in override_devices_per_worker:"},{"line_number":240,"context_line":"            options \u003d {}"},{"line_number":241,"context_line":"            options.update(override_options, override_devices\u003doverride_devices)"},{"line_number":242,"context_line":"            yield options"},{"line_number":243,"context_line":""},{"line_number":244,"context_line":"    def is_healthy(self):"},{"line_number":245,"context_line":"        \"\"\""}],"source_content_type":"text/x-python","patch_set":8,"id":"1f1a1f67_20ea1ed7","line":242,"updated":"2017-07-19 00:38:01.000000000","message":"nit: Shorter as\n\n yield dict(override_options, override_devices\u003doverride_devices)","commit_id":"52d92732cd19f73893f04738a89d8d9eaa2df4fd"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"e17013a69ef2bb33b061dccc3c4e5fa4e890392c","unresolved":false,"context_lines":[{"line_number":239,"context_line":"        for override_devices in override_devices_per_worker:"},{"line_number":240,"context_line":"            options \u003d {}"},{"line_number":241,"context_line":"            options.update(override_options, override_devices\u003doverride_devices)"},{"line_number":242,"context_line":"            yield options"},{"line_number":243,"context_line":""},{"line_number":244,"context_line":"    def is_healthy(self):"},{"line_number":245,"context_line":"        \"\"\""}],"source_content_type":"text/x-python","patch_set":8,"id":"1f1a1f67_dd5e4d44","line":242,"in_reply_to":"1f1a1f67_20ea1ed7","updated":"2017-07-21 19:10:19.000000000","message":"Done","commit_id":"52d92732cd19f73893f04738a89d8d9eaa2df4fd"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"c9fba230dfe4e041e6b434657c252159c14f2318","unresolved":false,"context_lines":[{"line_number":253,"context_line":"            self.aggregate_recon_update()"},{"line_number":254,"context_line":"        return all(self.check_ring(p.object_ring) for p in self.policies)"},{"line_number":255,"context_line":""},{"line_number":256,"context_line":"    def aggregate_recon_update(self):"},{"line_number":257,"context_line":"        try:"},{"line_number":258,"context_line":"            with open(self.rcache) as f:"},{"line_number":259,"context_line":"                existing_data \u003d json.load(f)"}],"source_content_type":"text/x-python","patch_set":8,"id":"1f1a1f67_80dd72e1","line":256,"updated":"2017-07-19 00:38:01.000000000","message":"So if self.reconstructor_workers is 0, (an approximation of) this gets handled by final_recon_dump down in run_once...","commit_id":"52d92732cd19f73893f04738a89d8d9eaa2df4fd"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"c9fba230dfe4e041e6b434657c252159c14f2318","unresolved":false,"context_lines":[{"line_number":275,"context_line":"            except KeyError:"},{"line_number":276,"context_line":"                continue"},{"line_number":277,"context_line":"            first_start \u003d min((first_start, start_time))"},{"line_number":278,"context_line":"            last_finish \u003d max((last_finish, finish_time))"},{"line_number":279,"context_line":"        if last_finish \u003e 0:"},{"line_number":280,"context_line":"            duration \u003d last_finish - first_start"},{"line_number":281,"context_line":"            recon_update \u003d {"}],"source_content_type":"text/x-python","patch_set":8,"id":"1f1a1f67_60013638","line":278,"updated":"2017-07-19 00:38:01.000000000","message":"nit: The extra tuple is unnecessary; this can just be\n\n first_start \u003d min(first_start, start_time)\n last_finish \u003d max(last_finish, finish_time)","commit_id":"52d92732cd19f73893f04738a89d8d9eaa2df4fd"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"e17013a69ef2bb33b061dccc3c4e5fa4e890392c","unresolved":false,"context_lines":[{"line_number":275,"context_line":"            except KeyError:"},{"line_number":276,"context_line":"                continue"},{"line_number":277,"context_line":"            first_start \u003d min((first_start, start_time))"},{"line_number":278,"context_line":"            last_finish \u003d max((last_finish, finish_time))"},{"line_number":279,"context_line":"        if last_finish \u003e 0:"},{"line_number":280,"context_line":"            duration \u003d last_finish - first_start"},{"line_number":281,"context_line":"            recon_update \u003d {"}],"source_content_type":"text/x-python","patch_set":8,"id":"1f1a1f67_b8371fe4","line":278,"in_reply_to":"1f1a1f67_60013638","updated":"2017-07-21 19:10:19.000000000","message":"Done","commit_id":"52d92732cd19f73893f04738a89d8d9eaa2df4fd"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"c9fba230dfe4e041e6b434657c252159c14f2318","unresolved":false,"context_lines":[{"line_number":280,"context_line":"            duration \u003d last_finish - first_start"},{"line_number":281,"context_line":"            recon_update \u003d {"},{"line_number":282,"context_line":"                \u0027object_reconstruction_time\u0027: duration / 60.0,"},{"line_number":283,"context_line":"                \u0027object_reconstruction_last\u0027: last_finish"},{"line_number":284,"context_line":"            }"},{"line_number":285,"context_line":"        else:"},{"line_number":286,"context_line":"            # if no current devices have yet dropped stats, or the rcache file"}],"source_content_type":"text/x-python","patch_set":8,"id":"1f1a1f67_0034c2fa","line":283,"updated":"2017-07-19 00:38:01.000000000","message":"So... as disks complete, we\u0027ll start writing object_reconstruction_time/object_reconstruction_last even though the cycle hasn\u0027t completed? This seems weird... like, I can peek at the stats for a *still running* reconstructor, and it claims it finished 5 minutes ago...\n\nNo, wait, 30 seconds ago...\n\nNo, wait, just now...\n\nAnd meanwhile my cycle time seems like it\u0027s pretty short, then grows and grows until the reconstructor *actually finishes* at which point I have, what, 30 seconds before it starts again?\n\nSeems like we want something like\n\n completed \u003d True\n for device in all_local_devices:\n     ...\n     except KeyError:\n         completed \u003d False\n         continue\n     ...\n\nand gate off completed instead of last_finish \u003e 0. If you *really* want to look at the in-progress cycle, look at the per-disk stats -- it\u0027ll give you a more complete picture, anyway.","commit_id":"52d92732cd19f73893f04738a89d8d9eaa2df4fd"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"c9fba230dfe4e041e6b434657c252159c14f2318","unresolved":false,"context_lines":[{"line_number":989,"context_line":"        # return a list of jobs for this part"},{"line_number":990,"context_line":"        return jobs"},{"line_number":991,"context_line":""},{"line_number":992,"context_line":"    def get_policy2devices(self):"},{"line_number":993,"context_line":"        ips \u003d whataremyips(self.bind_ip)"},{"line_number":994,"context_line":"        policy2devices \u003d {}"},{"line_number":995,"context_line":"        for policy in self.policies:"}],"source_content_type":"text/x-python","patch_set":8,"id":"1f1a1f67_e33310aa","line":992,"updated":"2017-07-19 00:38:01.000000000","message":"Worth memoizing? We hit this (usually by way of all_local_devices) in get_worker_args, aggregate_recon_update, final_recon_dump, and collect_parts, but it shouldn\u0027t really be changing -- that\u0027s what the ring checks are for, right?","commit_id":"52d92732cd19f73893f04738a89d8d9eaa2df4fd"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"c9fba230dfe4e041e6b434657c252159c14f2318","unresolved":false,"context_lines":[{"line_number":992,"context_line":"    def get_policy2devices(self):"},{"line_number":993,"context_line":"        ips \u003d whataremyips(self.bind_ip)"},{"line_number":994,"context_line":"        policy2devices \u003d {}"},{"line_number":995,"context_line":"        for policy in self.policies:"},{"line_number":996,"context_line":"            self.load_object_ring(policy)"},{"line_number":997,"context_line":"            local_devices \u003d list(six.moves.filter("},{"line_number":998,"context_line":"                lambda dev: dev and is_local_device("}],"source_content_type":"text/x-python","patch_set":8,"id":"1f1a1f67_6395c086","line":995,"updated":"2017-07-19 00:38:01.000000000","message":"Right, EC filtering now happens in __init__...","commit_id":"52d92732cd19f73893f04738a89d8d9eaa2df4fd"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"c9fba230dfe4e041e6b434657c252159c14f2318","unresolved":false,"context_lines":[{"line_number":1151,"context_line":"            self.run_pool \u003d GreenPool(size\u003dself.concurrency)"},{"line_number":1152,"context_line":"            for part_info in self.collect_parts(**kwargs):"},{"line_number":1153,"context_line":"                sleep()  # Give spawns a cycle"},{"line_number":1154,"context_line":"                if not self.check_ring(part_info[\u0027policy\u0027].object_ring):"},{"line_number":1155,"context_line":"                    self.logger.info(_(\"Ring change detected. Aborting \""},{"line_number":1156,"context_line":"                                       \"current reconstruction pass.\"))"},{"line_number":1157,"context_line":"                    return"}],"source_content_type":"text/x-python","patch_set":8,"id":"1f1a1f67_e6655eb6","line":1154,"updated":"2017-07-19 00:38:01.000000000","message":"Yeah, good -- still do the ring check even when workers\u003d0.","commit_id":"52d92732cd19f73893f04738a89d8d9eaa2df4fd"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"e17013a69ef2bb33b061dccc3c4e5fa4e890392c","unresolved":false,"context_lines":[{"line_number":1229,"context_line":"        # the existence of commandline overrides seems a somewhat weak"},{"line_number":1230,"context_line":"        # heuristic for if the operator wants a manual run to dump recon"},{"line_number":1231,"context_line":"        # stats over a background reconstructor"},{"line_number":1232,"context_line":"        if not any(override_options.values()):"},{"line_number":1233,"context_line":"            self.final_recon_dump(total, **override_options)"},{"line_number":1234,"context_line":""},{"line_number":1235,"context_line":"    def run_forever(self, *args, **kwargs):"}],"source_content_type":"text/x-python","patch_set":8,"id":"1f1a1f67_fecfbfad","line":1232,"updated":"2017-07-21 19:10:19.000000000","message":"previously dump_recon_cache is called if there are no command line overrides, but now it is never called in a worker process because override_options always has values (when there is work to do) - we need to distinguish between their being CLI overrides and get_worker_args generated overrides\n\nsee fix in https://review.openstack.org/486205","commit_id":"52d92732cd19f73893f04738a89d8d9eaa2df4fd"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"bc1f43ad7c56fc35685bf2bb75339c5da6c8edfb","unresolved":false,"context_lines":[{"line_number":276,"context_line":"                continue"},{"line_number":277,"context_line":"            first_start \u003d min((first_start, start_time))"},{"line_number":278,"context_line":"            last_finish \u003d max((last_finish, finish_time))"},{"line_number":279,"context_line":"        if last_finish \u003e 0:"},{"line_number":280,"context_line":"            duration \u003d last_finish - first_start"},{"line_number":281,"context_line":"            recon_update \u003d {"},{"line_number":282,"context_line":"                \u0027object_reconstruction_time\u0027: duration / 60.0,"}],"source_content_type":"text/x-python","patch_set":9,"id":"ff346bd7_26bd39b7","line":279,"updated":"2017-07-24 21:25:59.000000000","message":"OK, I\u0027m starting to get it...\n\nWorkers still shuffle all the parts for all the disks, so as *workers* finish (not disks as I originally was thinking), they write stats for *all the disks they handled, all at once*... which is why it\u0027s important that all the workers have roughly the same workload. Although, throw in some tarpit disks and the like, and who knows how that\u0027ll go...\n\nI\u0027m still a little nervous about our behavior during an upgrade, though:  when we switch to using workers, we\u0027ll start populating per-disk stats but won\u0027t have any history -- as a result, the *first* worker to complete makes it look like *the whole reconstruction cycle* has completed. I guess it\u0027s probably OK though? In the steady state, those workers eventually report, and cycle time jumps up to reflect any laggards.\n\nAlthough... if you had some automated tooling that pushed out rebalanced rings dependent on cycle times... you might get stuck in a bit of a loop. Consider a node with three disks and two workers:\n\n* The one-disk worker completes, writes per-disk stats\n* stats_interval passes, so parent Daemon writes aggregates\n* Auto-rebalancer sees that the cycle completed, pushes a new ring\n* Parent Daemon sees the ring change, kills \u0026 restarts both workers\n* Repeat\n\nIf you\u0027re being aggressive about your ring updates, that two-disk worker never gets a chance to write stats.\n\nMaybe it calls for some sort of migration? Before starting workers, open the recon file, check for per-disk stats, and if you don\u0027t find them, copy the last stats out for all the disks you know about? Or add a \"all disks reported\" flag\"?","commit_id":"c772746ceda028af306b624b743dfb42a1a85f41"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"bc1f43ad7c56fc35685bf2bb75339c5da6c8edfb","unresolved":false,"context_lines":[{"line_number":1228,"context_line":"            _(\"Object reconstruction complete (once). (%.02f minutes)\"), total)"},{"line_number":1229,"context_line":"        # the existence of commandline overrides seems a somewhat weak"},{"line_number":1230,"context_line":"        # heuristic for if the operator wants a manual run to dump recon"},{"line_number":1231,"context_line":"        # stats over a background reconstructor"},{"line_number":1232,"context_line":"        if not any(override_options.values()):"},{"line_number":1233,"context_line":"            self.final_recon_dump(total, **override_options)"},{"line_number":1234,"context_line":""}],"source_content_type":"text/x-python","patch_set":9,"id":"ff346bd7_76a9b679","line":1231,"updated":"2017-07-24 21:25:59.000000000","message":"It\u0027s not obvious to me that this is still desirable. I know we\u0027re preserving existing behavior, but I think we\u0027ve changed the assumptions underlying the old behavior. Now that we can do per-disk stats, I think we want something more like\n\n if (self.reconstructor_workers \u003e 0 and\n     not override_options[\u0027override_partitions]) or \\\n        not any(override_options.values()):\n     self.final_recon_dump(total, **override_options)","commit_id":"c772746ceda028af306b624b743dfb42a1a85f41"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"8b8a68cdf19bb7f8fd2bc604e0b1d82533f1de84","unresolved":false,"context_lines":[{"line_number":282,"context_line":"                finish_time \u003d per_disk_stats[\u0027object_reconstruction_last\u0027]"},{"line_number":283,"context_line":"            except KeyError:"},{"line_number":284,"context_line":"                all_devices_reporting \u003d False"},{"line_number":285,"context_line":"                continue"},{"line_number":286,"context_line":"            first_start \u003d min(first_start, start_time)"},{"line_number":287,"context_line":"            last_finish \u003d max(last_finish, finish_time)"},{"line_number":288,"context_line":"        if all_devices_reporting and last_finish \u003e 0:"}],"source_content_type":"text/x-python","patch_set":10,"id":"ff346bd7_1395b540","line":285,"range":{"start_line":285,"start_character":16,"end_line":285,"end_character":24},"updated":"2017-07-25 17:22:20.000000000","message":"could be a break ?","commit_id":"f7eb5af2b62d3a9dfe2b20c47da8dd2d5144326d"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"8b8a68cdf19bb7f8fd2bc604e0b1d82533f1de84","unresolved":false,"context_lines":[{"line_number":292,"context_line":"                \u0027object_reconstruction_last\u0027: last_finish"},{"line_number":293,"context_line":"            }"},{"line_number":294,"context_line":"        else:"},{"line_number":295,"context_line":"            # if no current devices have yet dropped stats, or the rcache file"},{"line_number":296,"context_line":"            # does not yet exist, we may still clear out per device stats for"},{"line_number":297,"context_line":"            # any devices that have been removed from local devices"},{"line_number":298,"context_line":"            recon_update \u003d {}"}],"source_content_type":"text/x-python","patch_set":10,"id":"ff346bd7_53dd7d40","line":295,"range":{"start_line":295,"start_character":17,"end_line":295,"end_character":19},"updated":"2017-07-25 17:22:20.000000000","message":"s/no/not all/","commit_id":"f7eb5af2b62d3a9dfe2b20c47da8dd2d5144326d"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"8b8a68cdf19bb7f8fd2bc604e0b1d82533f1de84","unresolved":false,"context_lines":[{"line_number":1236,"context_line":"        total \u003d (time.time() - start) / 60"},{"line_number":1237,"context_line":"        self.logger.info("},{"line_number":1238,"context_line":"            _(\"Object reconstruction complete (once). (%.02f minutes)\"), total)"},{"line_number":1239,"context_line":"        # Only dump stats if they would actually be meaningful -- i.e. we\u0027re"},{"line_number":1240,"context_line":"        # collecting per-disk stats and covering all partitions, or we\u0027re"},{"line_number":1241,"context_line":"        # covering all partitions, all disks."},{"line_number":1242,"context_line":"        if not override_options[\u0027override_partitions\u0027] and ("},{"line_number":1243,"context_line":"                self.reconstructor_workers \u003e 0 or"},{"line_number":1244,"context_line":"                not override_options[\u0027override_devices\u0027]):"},{"line_number":1245,"context_line":"            self.final_recon_dump(total, **override_options)"}],"source_content_type":"text/x-python","patch_set":10,"id":"ff346bd7_691d3072","line":1242,"range":{"start_line":1239,"start_character":0,"end_line":1242,"end_character":1},"updated":"2017-07-25 17:22:20.000000000","message":"I\u0027m a little nervous about this but I think it\u0027s ok because:\n\n- run_once inline with override is unchanged - I guess the original intent was to NOT overwrite aggregate stats for *all* devices when running once with *subset* of devices\n- you have to explicitly enable multiple workers ptherwise behaviour is as before\n- run_once in a worker with override will only dump per-disk stats so does not unilaterally overwrite the aggregate stats , and if run_forever is in progress too then the aggregation will just roll the run_once results up with all other devices.\n- I agree that if only a subset of partitions is selected by override option then the stats should not be dumped - that could cause a misleading aggregate when rolled up.\n\nI haven\u0027t yet found anywhere that documents the existing behaviour.","commit_id":"f7eb5af2b62d3a9dfe2b20c47da8dd2d5144326d"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"3125cb76246f647312b67185b8ef46b7f60b4e91","unresolved":false,"context_lines":[{"line_number":1250,"context_line":"        while True:"},{"line_number":1251,"context_line":"            start \u003d time.time()"},{"line_number":1252,"context_line":"            self.logger.info(_(\"Starting object reconstruction pass.\"))"},{"line_number":1253,"context_line":"            override_options \u003d parse_override_options(**kwargs)"},{"line_number":1254,"context_line":"            # Run the reconstructor"},{"line_number":1255,"context_line":"            self.reconstruct(**override_options)"},{"line_number":1256,"context_line":"            total \u003d (time.time() - start) / 60"}],"source_content_type":"text/x-python","patch_set":11,"id":"ff346bd7_0fac619e","line":1253,"range":{"start_line":1253,"start_character":12,"end_line":1253,"end_character":63},"updated":"2017-07-25 19:13:13.000000000","message":"before, CLI override options only took effect in run_once mode (for replicator this is documented here [1], its reasonable to infer same for reconstructor, and that was the behavior)\n\nNow, CLI overrides affect run_forever. Apart from being a change in bahvior, the recon dump aggregation can become quite strange with CLI overrides and run_forever\n\n- if there has been no previous stats dumped, then there will never be an aggregation if not all devices are selected by a CLI override\n- if previous stats include a device that is not now in the CLI overrides, then its historic stats get included in a an aggregate result which causes the apparent last_time to get longer and longer.\n\n[1] https://docs.openstack.org/swift/latest/admin_guide.html#object-replicator","commit_id":"8e98ef43fb6f212f80ba37a0e22e3239f5d162ee"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"f309410a9e1be45f9b1c63d6c20fb765bedb28ce","unresolved":false,"context_lines":[{"line_number":270,"context_line":"        if now \u003e self._next_rcache_update:"},{"line_number":271,"context_line":"            self._next_rcache_update \u003d now + self.stats_interval"},{"line_number":272,"context_line":"            self.aggregate_recon_update()"},{"line_number":273,"context_line":"        return all(self.check_ring(p.object_ring) for p in self.policies)"},{"line_number":274,"context_line":""},{"line_number":275,"context_line":"    def aggregate_recon_update(self):"},{"line_number":276,"context_line":"        \"\"\""}],"source_content_type":"text/x-python","patch_set":12,"id":"ff346bd7_284a5fab","line":273,"range":{"start_line":273,"start_character":24,"end_line":273,"end_character":34},"updated":"2017-07-26 14:30:19.000000000","message":"In practice I found that when I changed my ring (a) the is_healthy check might not fire or (b) if it did the worker args might not change. I think that is because:\n\n(a) check_ring calls Ring.has_changed() which checks if the ring file has changed since *the Ring last reloaded itself*, but the ring reloads itself if it sees a file change dor example when calling ring.devs. That means that before check_ring calls has_changed, the ring has reloaded itself and rest its mtime, so now thinks it hasn\u0027t changed.\n\n(b) if is_healthy does return False (i.e. this happens to be the first call to ring.hash_changed() since the mtime changed), so the daemon calls get_worker_args, calling policy.load_ring() does not reload the ring! So get_worker_args just returns same results as before (unless by chance something else causes the rings to reload between here and when get_worker_args - unlikely)","commit_id":"a4499a45fc638f2a3d0530d9ed098821964ea2c3"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"f309410a9e1be45f9b1c63d6c20fb765bedb28ce","unresolved":false,"context_lines":[{"line_number":337,"context_line":"        :returns: boolean indicating whether or not the ring has changed"},{"line_number":338,"context_line":"        \"\"\""},{"line_number":339,"context_line":"        if time.time() \u003e self.next_check:"},{"line_number":340,"context_line":"            self.next_check \u003d time.time() + self.ring_check_interval"},{"line_number":341,"context_line":"            if object_ring.has_changed():"},{"line_number":342,"context_line":"                return False"},{"line_number":343,"context_line":"        return True"}],"source_content_type":"text/x-python","patch_set":12,"id":"ff346bd7_888a6bbc","line":340,"updated":"2017-07-26 14:30:19.000000000","message":"hmmm, next_check is incremented per ring - so for larger number of rings the interval between checking can be much larger than the config\u0027d value? In fact is it possibe that some rings never get checked if the order of calling this is deterministic - first ring advance next_check, subsequent rings have to wait, but then first ring advance next_check... (haven\u0027t tested that)","commit_id":"a4499a45fc638f2a3d0530d9ed098821964ea2c3"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"2a7b696199f166f6d857416527c60f867a68c3d8","unresolved":false,"context_lines":[{"line_number":274,"context_line":"        if now \u003e self._next_rcache_update:"},{"line_number":275,"context_line":"            self._next_rcache_update \u003d now + self.stats_interval"},{"line_number":276,"context_line":"            self.aggregate_recon_update()"},{"line_number":277,"context_line":"        return self.get_local_devices() \u003d\u003d self.all_local_devices"},{"line_number":278,"context_line":""},{"line_number":279,"context_line":"    def aggregate_recon_update(self):"},{"line_number":280,"context_line":"        \"\"\""}],"source_content_type":"text/x-python","patch_set":13,"id":"ff346bd7_f93c0430","line":277,"updated":"2017-07-27 14:39:09.000000000","message":"nit: is_healthy is called *often* (~10 per sec) but the ring devices may not change for up to 15 secs after the ring file changed. We could perform this check every self.ring_check_interval, or if we really care about this being reponsive then we need it to force an immediate ring reload after change.","commit_id":"701a172afac37229b85ea762f20428f6f422d29b"}],"test/unit/common/test_daemon.py":[{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"861dccd4c572ed6ebb3aa936a305b12aca24cf19","unresolved":false,"context_lines":[{"line_number":14,"context_line":"# limitations under the License."},{"line_number":15,"context_line":""},{"line_number":16,"context_line":"# TODO(clayg): Test kill_children signal handlers"},{"line_number":17,"context_line":""},{"line_number":18,"context_line":"import os"},{"line_number":19,"context_line":"from six import StringIO"},{"line_number":20,"context_line":"from six.moves import reload_module"}],"source_content_type":"text/x-python","patch_set":2,"id":"3f1d235d_3a3d672c","side":"PARENT","line":17,"updated":"2017-07-08 01:27:09.000000000","message":"as best I can tell this got tested at some point...","commit_id":"37a1935198fd9bbbbf592760e31cc60774befefb"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"737b1413e75c300359d6dd886e7b0e848bef2ba1","unresolved":false,"context_lines":[{"line_number":68,"context_line":""},{"line_number":69,"context_line":"class MyWorkerDaemon(MyDaemon):"},{"line_number":70,"context_line":""},{"line_number":71,"context_line":"    @property"},{"line_number":72,"context_line":"    def workers(self):"},{"line_number":73,"context_line":"        return int(self.conf.get(\u0027workers\u0027, 0))"},{"line_number":74,"context_line":""},{"line_number":75,"context_line":"    def create_workers(self, **kwargs):"},{"line_number":76,"context_line":"        return [kwargs for i in range(self.workers)]"}],"source_content_type":"text/x-python","patch_set":5,"id":"1f1a1f67_d3517064","line":73,"range":{"start_line":71,"start_character":1,"end_line":73,"end_character":47},"updated":"2017-07-14 19:50:11.000000000","message":"not needed I assume","commit_id":"fcd24235da12b2bca8dbd550febe68723d593ba1"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"1dc83b5363aa66bc58f8f7c886e21df276f81588","unresolved":false,"context_lines":[{"line_number":70,"context_line":""},{"line_number":71,"context_line":"    @property"},{"line_number":72,"context_line":"    def workers(self):"},{"line_number":73,"context_line":"        return int(self.conf.get(\u0027workers\u0027, 0))"},{"line_number":74,"context_line":""},{"line_number":75,"context_line":"    def create_workers(self, **kwargs):"},{"line_number":76,"context_line":"        return [kwargs for i in range(self.workers)]"}],"source_content_type":"text/x-python","patch_set":5,"id":"1f1a1f67_f8d42522","line":73,"in_reply_to":"1f1a1f67_d3517064","updated":"2017-07-17 05:15:23.000000000","message":"was used in create_workers, but probably distracting since it\u0027s not part of the interface anymore...","commit_id":"fcd24235da12b2bca8dbd550febe68723d593ba1"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"c9fba230dfe4e041e6b434657c252159c14f2318","unresolved":false,"context_lines":[{"line_number":69,"context_line":"class MyWorkerDaemon(MyDaemon):"},{"line_number":70,"context_line":""},{"line_number":71,"context_line":"    def get_worker_args(self, **kwargs):"},{"line_number":72,"context_line":"        return [kwargs for i in range(int(self.conf.get(\u0027workers\u0027, 0)))]"},{"line_number":73,"context_line":""},{"line_number":74,"context_line":"    def is_healthy(self):"},{"line_number":75,"context_line":"        try:"}],"source_content_type":"text/x-python","patch_set":8,"id":"1f1a1f67_03172ca6","line":72,"updated":"2017-07-19 00:38:01.000000000","message":"nit:\n\n return [kwargs] * int(self.conf.get(\u0027workers\u0027, 0))\n\n? Eh, *shrug*","commit_id":"52d92732cd19f73893f04738a89d8d9eaa2df4fd"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"c9fba230dfe4e041e6b434657c252159c14f2318","unresolved":false,"context_lines":[{"line_number":92,"context_line":"        self.assertEqual([], d.get_worker_args())"},{"line_number":93,"context_line":"        self.assertTrue(d.is_healthy())"},{"line_number":94,"context_line":"        d \u003d MyWorkerDaemon({\u0027workers\u0027: \u00273\u0027})"},{"line_number":95,"context_line":"        self.assertEqual([{}] * 3, d.get_worker_args())"},{"line_number":96,"context_line":"        d.health_side_effects \u003d [True, False]"},{"line_number":97,"context_line":"        self.assertTrue(d.is_healthy())"},{"line_number":98,"context_line":"        self.assertFalse(d.is_healthy())"}],"source_content_type":"text/x-python","patch_set":8,"id":"1f1a1f67_23f7c85b","line":95,"range":{"start_line":95,"start_character":37,"end_line":95,"end_character":54},"updated":"2017-07-19 00:38:01.000000000","message":"Worth actually passing some kwargs and seeing them plumbed through?","commit_id":"52d92732cd19f73893f04738a89d8d9eaa2df4fd"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"e17013a69ef2bb33b061dccc3c4e5fa4e890392c","unresolved":false,"context_lines":[{"line_number":92,"context_line":"        self.assertEqual([], d.get_worker_args())"},{"line_number":93,"context_line":"        self.assertTrue(d.is_healthy())"},{"line_number":94,"context_line":"        d \u003d MyWorkerDaemon({\u0027workers\u0027: \u00273\u0027})"},{"line_number":95,"context_line":"        self.assertEqual([{}] * 3, d.get_worker_args())"},{"line_number":96,"context_line":"        d.health_side_effects \u003d [True, False]"},{"line_number":97,"context_line":"        self.assertTrue(d.is_healthy())"},{"line_number":98,"context_line":"        self.assertFalse(d.is_healthy())"}],"source_content_type":"text/x-python","patch_set":8,"id":"1f1a1f67_b70cc542","line":95,"range":{"start_line":95,"start_character":37,"end_line":95,"end_character":54},"in_reply_to":"1f1a1f67_23f7c85b","updated":"2017-07-21 19:10:19.000000000","message":"Done","commit_id":"52d92732cd19f73893f04738a89d8d9eaa2df4fd"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"c9fba230dfe4e041e6b434657c252159c14f2318","unresolved":false,"context_lines":[{"line_number":221,"context_line":"            strategy.run(once\u003dTrue)"},{"line_number":222,"context_line":"        self.assertEqual([mock.call()] * 3, self.mock_fork.call_args_list)"},{"line_number":223,"context_line":"        self.assertEqual(self.waitpid_calls, {"},{"line_number":224,"context_line":"            \u0027mock-pid-0\u0027: 3,"},{"line_number":225,"context_line":"            \u0027mock-pid-1\u0027: 3,"},{"line_number":226,"context_line":"            \u0027mock-pid-2\u0027: 3,"},{"line_number":227,"context_line":"        })"}],"source_content_type":"text/x-python","patch_set":8,"id":"1f1a1f67_a3475877","line":224,"range":{"start_line":224,"start_character":26,"end_line":224,"end_character":27},"updated":"2017-07-19 00:38:01.000000000","message":"This comes from the default value for child_worker_cycles, rather than the workers config value, yeah?","commit_id":"52d92732cd19f73893f04738a89d8d9eaa2df4fd"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"e17013a69ef2bb33b061dccc3c4e5fa4e890392c","unresolved":false,"context_lines":[{"line_number":221,"context_line":"            strategy.run(once\u003dTrue)"},{"line_number":222,"context_line":"        self.assertEqual([mock.call()] * 3, self.mock_fork.call_args_list)"},{"line_number":223,"context_line":"        self.assertEqual(self.waitpid_calls, {"},{"line_number":224,"context_line":"            \u0027mock-pid-0\u0027: 3,"},{"line_number":225,"context_line":"            \u0027mock-pid-1\u0027: 3,"},{"line_number":226,"context_line":"            \u0027mock-pid-2\u0027: 3,"},{"line_number":227,"context_line":"        })"}],"source_content_type":"text/x-python","patch_set":8,"id":"1f1a1f67_bb36efb3","line":224,"range":{"start_line":224,"start_character":26,"end_line":224,"end_character":27},"in_reply_to":"1f1a1f67_a3475877","updated":"2017-07-21 19:10:19.000000000","message":"Agree - assertion is that each worker got checked 3 times via waitpid","commit_id":"52d92732cd19f73893f04738a89d8d9eaa2df4fd"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"c9fba230dfe4e041e6b434657c252159c14f2318","unresolved":false,"context_lines":[{"line_number":224,"context_line":"            \u0027mock-pid-0\u0027: 3,"},{"line_number":225,"context_line":"            \u0027mock-pid-1\u0027: 3,"},{"line_number":226,"context_line":"            \u0027mock-pid-2\u0027: 3,"},{"line_number":227,"context_line":"        })"},{"line_number":228,"context_line":""},{"line_number":229,"context_line":"    def test_forked_worker(self):"},{"line_number":230,"context_line":"        d \u003d MyWorkerDaemon({\u0027workers\u0027: 3})"}],"source_content_type":"text/x-python","patch_set":8,"id":"1f1a1f67_e31f9069","line":227,"updated":"2017-07-19 00:38:01.000000000","message":"Maybe add\n\n self.assertEqual([], self.mock_kill.call_args_list)","commit_id":"52d92732cd19f73893f04738a89d8d9eaa2df4fd"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"e17013a69ef2bb33b061dccc3c4e5fa4e890392c","unresolved":false,"context_lines":[{"line_number":224,"context_line":"            \u0027mock-pid-0\u0027: 3,"},{"line_number":225,"context_line":"            \u0027mock-pid-1\u0027: 3,"},{"line_number":226,"context_line":"            \u0027mock-pid-2\u0027: 3,"},{"line_number":227,"context_line":"        })"},{"line_number":228,"context_line":""},{"line_number":229,"context_line":"    def test_forked_worker(self):"},{"line_number":230,"context_line":"        d \u003d MyWorkerDaemon({\u0027workers\u0027: 3})"}],"source_content_type":"text/x-python","patch_set":8,"id":"1f1a1f67_fbb7c739","line":227,"in_reply_to":"1f1a1f67_e31f9069","updated":"2017-07-21 19:10:19.000000000","message":"Done","commit_id":"52d92732cd19f73893f04738a89d8d9eaa2df4fd"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"e17013a69ef2bb33b061dccc3c4e5fa4e890392c","unresolved":false,"context_lines":[{"line_number":299,"context_line":"        with mock.patch(\u0027swift.common.daemon.os.waitpid\u0027) as mock_waitpid:"},{"line_number":300,"context_line":"            mock_waitpid.side_effect \u003d OSError("},{"line_number":301,"context_line":"                errno.ECHILD, os.strerror(errno.ECHILD))"},{"line_number":302,"context_line":"            mock_waitpid.return_value \u003d (\u0027mock-pid\u0027, 0)"},{"line_number":303,"context_line":"            strategy.check_on_all_running_workers()"},{"line_number":304,"context_line":"        self.assertEqual(strategy.unspawned_worker_options, ["},{"line_number":305,"context_line":"            {\u0027mock_options\u0027: True}])"}],"source_content_type":"text/x-python","patch_set":8,"id":"1f1a1f67_be4c9d76","line":302,"range":{"start_line":302,"start_character":12,"end_line":302,"end_character":55},"updated":"2017-07-21 19:10:19.000000000","message":"nit: not necessary given the side_effect?","commit_id":"52d92732cd19f73893f04738a89d8d9eaa2df4fd"}],"test/unit/obj/test_reconstructor.py":[{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"e994ec0ec2a8f39afb920127a793d53d83719a94","unresolved":false,"context_lines":[{"line_number":1543,"context_line":"        self.assertEqual({"},{"line_number":1544,"context_line":"            \u0027object_reconstruction_last\u0027: now,"},{"line_number":1545,"context_line":"            \u0027object_reconstruction_time\u0027: total,"},{"line_number":1546,"context_line":"            # put_recon_cache_entry is not idempotent"},{"line_number":1547,"context_line":"            \u0027object_reconstruction_per_disk\u0027: {}"},{"line_number":1548,"context_line":"        }, data)"},{"line_number":1549,"context_line":"        total \u003d 14.0"}],"source_content_type":"text/x-python","patch_set":7,"id":"1f1a1f67_9bfbe677","line":1546,"updated":"2017-07-17 19:51:24.000000000","message":"in fact it toggles empty data sets, so here we have \u0027object_reconstruction_per_disk\u0027 key and at line 1555 we don\u0027t","commit_id":"40ce897e94585cf545e23f320f72cb18f1d1035a"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"3a194b43219c6495f4aa98252484163f415e40e2","unresolved":false,"context_lines":[{"line_number":1543,"context_line":"        self.assertEqual({"},{"line_number":1544,"context_line":"            \u0027object_reconstruction_last\u0027: now,"},{"line_number":1545,"context_line":"            \u0027object_reconstruction_time\u0027: total,"},{"line_number":1546,"context_line":"            # put_recon_cache_entry is not idempotent"},{"line_number":1547,"context_line":"            \u0027object_reconstruction_per_disk\u0027: {}"},{"line_number":1548,"context_line":"        }, data)"},{"line_number":1549,"context_line":"        total \u003d 14.0"}],"source_content_type":"text/x-python","patch_set":7,"id":"1f1a1f67_9cbf4560","line":1546,"in_reply_to":"1f1a1f67_9bfbe677","updated":"2017-07-18 00:21:13.000000000","message":"Done","commit_id":"40ce897e94585cf545e23f320f72cb18f1d1035a"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"e994ec0ec2a8f39afb920127a793d53d83719a94","unresolved":false,"context_lines":[{"line_number":1622,"context_line":"        self.assertEqual({"},{"line_number":1623,"context_line":"            \u0027object_reconstruction_last\u0027: later,"},{"line_number":1624,"context_line":"            \u0027object_reconstruction_time\u0027: 5.0,"},{"line_number":1625,"context_line":"            # put_recon_cache_entry is not idempotent"},{"line_number":1626,"context_line":"            \u0027object_reconstruction_per_disk\u0027: {}"},{"line_number":1627,"context_line":"        }, data)"},{"line_number":1628,"context_line":"        total \u003d 10.0"}],"source_content_type":"text/x-python","patch_set":7,"id":"1f1a1f67_1b65961e","line":1625,"updated":"2017-07-17 19:51:24.000000000","message":"hmm, so this is \u0027legacy mode\u0027 but we get a new key in recon cache - feels like dump_recon_cache is doing the wrong thing with the empty dict value","commit_id":"40ce897e94585cf545e23f320f72cb18f1d1035a"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"3a194b43219c6495f4aa98252484163f415e40e2","unresolved":false,"context_lines":[{"line_number":1622,"context_line":"        self.assertEqual({"},{"line_number":1623,"context_line":"            \u0027object_reconstruction_last\u0027: later,"},{"line_number":1624,"context_line":"            \u0027object_reconstruction_time\u0027: 5.0,"},{"line_number":1625,"context_line":"            # put_recon_cache_entry is not idempotent"},{"line_number":1626,"context_line":"            \u0027object_reconstruction_per_disk\u0027: {}"},{"line_number":1627,"context_line":"        }, data)"},{"line_number":1628,"context_line":"        total \u003d 10.0"}],"source_content_type":"text/x-python","patch_set":7,"id":"1f1a1f67_bc8c41d7","line":1625,"in_reply_to":"1f1a1f67_1b65961e","updated":"2017-07-18 00:21:13.000000000","message":"Done","commit_id":"40ce897e94585cf545e23f320f72cb18f1d1035a"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"e994ec0ec2a8f39afb920127a793d53d83719a94","unresolved":false,"context_lines":[{"line_number":1833,"context_line":"        with open(self.rcache) as f:"},{"line_number":1834,"context_line":"            data \u003d json.load(f)"},{"line_number":1835,"context_line":"        self.assertEqual(start + 900, data[\u0027object_reconstruction_last\u0027])"},{"line_number":1836,"context_line":"        self.assertAlmostEqual(15, data[\u0027object_reconstruction_time\u0027],"},{"line_number":1837,"context_line":"                               places\u003d3)"},{"line_number":1838,"context_line":"        self.assertEqual(set([\u0027d0\u0027, \u0027d1\u0027, \u0027d2\u0027, \u0027d3\u0027]),"},{"line_number":1839,"context_line":"                         set(data[\u0027object_reconstruction_per_disk\u0027].keys()))"}],"source_content_type":"text/x-python","patch_set":7,"id":"1f1a1f67_de14a246","line":1836,"range":{"start_line":1836,"start_character":13,"end_line":1836,"end_character":30},"updated":"2017-07-17 19:51:24.000000000","message":"does this need to be \u0027almost\u0027 equal? - elsewhere we test for exact equality e.g. line 1847","commit_id":"40ce897e94585cf545e23f320f72cb18f1d1035a"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"3a194b43219c6495f4aa98252484163f415e40e2","unresolved":false,"context_lines":[{"line_number":1833,"context_line":"        with open(self.rcache) as f:"},{"line_number":1834,"context_line":"            data \u003d json.load(f)"},{"line_number":1835,"context_line":"        self.assertEqual(start + 900, data[\u0027object_reconstruction_last\u0027])"},{"line_number":1836,"context_line":"        self.assertAlmostEqual(15, data[\u0027object_reconstruction_time\u0027],"},{"line_number":1837,"context_line":"                               places\u003d3)"},{"line_number":1838,"context_line":"        self.assertEqual(set([\u0027d0\u0027, \u0027d1\u0027, \u0027d2\u0027, \u0027d3\u0027]),"},{"line_number":1839,"context_line":"                         set(data[\u0027object_reconstruction_per_disk\u0027].keys()))"}],"source_content_type":"text/x-python","patch_set":7,"id":"1f1a1f67_fc8639b9","line":1836,"in_reply_to":"1f1a1f67_de14a246","updated":"2017-07-18 00:21:13.000000000","message":"Done","commit_id":"40ce897e94585cf545e23f320f72cb18f1d1035a"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"c9fba230dfe4e041e6b434657c252159c14f2318","unresolved":false,"context_lines":[{"line_number":1678,"context_line":"                self.assertRaises(StopForever):"},{"line_number":1679,"context_line":"            gt \u003d spawn(reconstructor.run_forever, override_devices\u003d["},{"line_number":1680,"context_line":"                \u0027sda\u0027, \u0027sdb\u0027])"},{"line_number":1681,"context_line":"            gt.wait()"},{"line_number":1682,"context_line":"        # override args are passed to reconstruct"},{"line_number":1683,"context_line":"        self.assertEqual([mock.call("},{"line_number":1684,"context_line":"            override_devices\u003d[\u0027sda\u0027, \u0027sdb\u0027],"}],"source_content_type":"text/x-python","patch_set":8,"id":"1f1a1f67_06505a0a","line":1681,"updated":"2017-07-19 00:38:01.000000000","message":"Why bother spawning if we\u0027re immediately going to wait? Should we have a Timeout on here or something?","commit_id":"52d92732cd19f73893f04738a89d8d9eaa2df4fd"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"c9fba230dfe4e041e6b434657c252159c14f2318","unresolved":false,"context_lines":[{"line_number":1716,"context_line":"                self.assertRaises(StopForever):"},{"line_number":1717,"context_line":"            gt \u003d spawn(reconstructor.run_forever, override_devices\u003d["},{"line_number":1718,"context_line":"                \u0027sdc\u0027, \u0027sdd\u0027])"},{"line_number":1719,"context_line":"            gt.wait()"},{"line_number":1720,"context_line":"        # override args are parsed"},{"line_number":1721,"context_line":"        self.assertEqual([mock.call("},{"line_number":1722,"context_line":"            override_devices\u003d[\u0027sdc\u0027, \u0027sdd\u0027],"}],"source_content_type":"text/x-python","patch_set":8,"id":"1f1a1f67_c655c2fb","line":1719,"updated":"2017-07-19 00:38:01.000000000","message":"Similarly here... why spawn/wait?","commit_id":"52d92732cd19f73893f04738a89d8d9eaa2df4fd"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"bc1f43ad7c56fc35685bf2bb75339c5da6c8edfb","unresolved":false,"context_lines":[{"line_number":1373,"context_line":"        reconstructor \u003d object_reconstructor.ObjectReconstructor("},{"line_number":1374,"context_line":"            {\u0027reconstructor_workers\u0027: \u0027-1\u0027}, logger\u003dself.logger)"},{"line_number":1375,"context_line":"        self.assertEqual(-1, reconstructor.reconstructor_workers)"},{"line_number":1376,"context_line":"        self.assertEqual(0, len(list(reconstructor.get_worker_args())))"},{"line_number":1377,"context_line":""},{"line_number":1378,"context_line":"    def test_workers_with_no_devices(self):"},{"line_number":1379,"context_line":"        def do_test(num_workers):"}],"source_content_type":"text/x-python","patch_set":9,"id":"ff346bd7_5cedb0ed","line":1376,"updated":"2017-07-24 21:25:59.000000000","message":"Maybe better to ValueError, like if you tried to have \u00271.1\u0027 workers?","commit_id":"c772746ceda028af306b624b743dfb42a1a85f41"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"bc1f43ad7c56fc35685bf2bb75339c5da6c8edfb","unresolved":false,"context_lines":[{"line_number":1993,"context_line":"            data \u003d json.load(f)"},{"line_number":1994,"context_line":"        self.assertEqual({"},{"line_number":1995,"context_line":"            \u0027object_reconstruction_last\u0027: start,"},{"line_number":1996,"context_line":"            \u0027object_reconstruction_time\u0027: 1,  # d1 stats not in aggregate yet"},{"line_number":1997,"context_line":"            \u0027object_reconstruction_per_disk\u0027: {"},{"line_number":1998,"context_line":"                \u0027d0\u0027: {"},{"line_number":1999,"context_line":"                    \u0027object_reconstruction_last\u0027: start,"}],"source_content_type":"text/x-python","patch_set":9,"id":"ff346bd7_3ca1ccaa","line":1996,"updated":"2017-07-24 21:25:59.000000000","message":"This part seems dangerous -- existing tooling will think we\u0027re done already! Shouldn\u0027t we wait until all disks are reporting?","commit_id":"c772746ceda028af306b624b743dfb42a1a85f41"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"8b8a68cdf19bb7f8fd2bc604e0b1d82533f1de84","unresolved":false,"context_lines":[{"line_number":1397,"context_line":"        worker_args \u003d list(reconstructor.get_worker_args(devices\u003d\u0027sdz\u0027))"},{"line_number":1398,"context_line":"        self.assertEqual(1, len(worker_args))"},{"line_number":1399,"context_line":"        self.assertEqual([{\u0027override_partitions\u0027: [],"},{"line_number":1400,"context_line":"                           \u0027override_devices\u0027: []}],"},{"line_number":1401,"context_line":"                         worker_args)"},{"line_number":1402,"context_line":""},{"line_number":1403,"context_line":"    def test_workers_with_devices(self):"}],"source_content_type":"text/x-python","patch_set":10,"id":"ff346bd7_5800fc67","line":1400,"range":{"start_line":1400,"start_character":47,"end_line":1400,"end_character":49},"updated":"2017-07-25 17:22:20.000000000","message":"should be \u0027sdz\u0027 - we need to pass the bogus device to reconstruct otherwise reconstruct will process *all* devices","commit_id":"f7eb5af2b62d3a9dfe2b20c47da8dd2d5144326d"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"8b8a68cdf19bb7f8fd2bc604e0b1d82533f1de84","unresolved":false,"context_lines":[{"line_number":1742,"context_line":"            \u0027object_reconstruction_time\u0027: 10.0,"},{"line_number":1743,"context_line":"        }, data)"},{"line_number":1744,"context_line":""},{"line_number":1745,"context_line":"    def test_no_dump_recon_run_once_with_partitions(self):"},{"line_number":1746,"context_line":"        reconstructor \u003d object_reconstructor.ObjectReconstructor("},{"line_number":1747,"context_line":"            {\u0027recon_cache_path\u0027: self.recon_cache_path,"},{"line_number":1748,"context_line":"             \u0027reconstructor_workers\u0027: 1},"}],"source_content_type":"text/x-python","patch_set":10,"id":"ff346bd7_154b5d2b","line":1745,"range":{"start_line":1745,"start_character":36,"end_line":1745,"end_character":51},"updated":"2017-07-25 17:22:20.000000000","message":"I may revert this because once the inline cases are tested realistically then the \u0027with_partitions\u0027 piece is not uniformly applicable - run_once inline with only override devices also does not dump recon.","commit_id":"f7eb5af2b62d3a9dfe2b20c47da8dd2d5144326d"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"8b8a68cdf19bb7f8fd2bc604e0b1d82533f1de84","unresolved":false,"context_lines":[{"line_number":1745,"context_line":"    def test_no_dump_recon_run_once_with_partitions(self):"},{"line_number":1746,"context_line":"        reconstructor \u003d object_reconstructor.ObjectReconstructor("},{"line_number":1747,"context_line":"            {\u0027recon_cache_path\u0027: self.recon_cache_path,"},{"line_number":1748,"context_line":"             \u0027reconstructor_workers\u0027: 1},"},{"line_number":1749,"context_line":"            logger\u003dself.logger)"},{"line_number":1750,"context_line":"        reconstructor.all_local_devices \u003d lambda: {\u0027sda\u0027, \u0027sdb\u0027, \u0027sdc\u0027}"},{"line_number":1751,"context_line":""}],"source_content_type":"text/x-python","patch_set":10,"id":"ff346bd7_d245ee16","line":1748,"range":{"start_line":1748,"start_character":13,"end_line":1748,"end_character":39},"updated":"2017-07-25 17:22:20.000000000","message":"with the change to run_once to inspect reconstructor_workers, this option shouldn\u0027t be set for the inline mode test scenarios","commit_id":"f7eb5af2b62d3a9dfe2b20c47da8dd2d5144326d"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"8b8a68cdf19bb7f8fd2bc604e0b1d82533f1de84","unresolved":false,"context_lines":[{"line_number":1750,"context_line":"        reconstructor.all_local_devices \u003d lambda: {\u0027sda\u0027, \u0027sdb\u0027, \u0027sdc\u0027}"},{"line_number":1751,"context_line":""},{"line_number":1752,"context_line":"        def do_test(run_once_kwargs, expected_devices, expected_partitions,"},{"line_number":1753,"context_line":"                    expect_recon_cache):"},{"line_number":1754,"context_line":"            reconstructor.reconstruct \u003d mock.MagicMock()"},{"line_number":1755,"context_line":"            now \u003d time.time()"},{"line_number":1756,"context_line":"            later \u003d now + 300  # 5 mins"}],"source_content_type":"text/x-python","patch_set":10,"id":"ff346bd7_f592199f","line":1753,"range":{"start_line":1753,"start_character":20,"end_line":1753,"end_character":38},"updated":"2017-07-25 17:22:20.000000000","message":"I\u0027d prefer to test all the cases where recon cache *is* dumped separately in the previous test so we also make assertions about *what* gets dumped (and to make more sense of the test method name)","commit_id":"f7eb5af2b62d3a9dfe2b20c47da8dd2d5144326d"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"8b8a68cdf19bb7f8fd2bc604e0b1d82533f1de84","unresolved":false,"context_lines":[{"line_number":1771,"context_line":"        # for each scenario check using inline mode or single worker process"},{"line_number":1772,"context_line":"        kwargs \u003d {\u0027devices\u0027: \u0027sda,sdb\u0027}"},{"line_number":1773,"context_line":"        worker_kwargs \u003d list(reconstructor.get_worker_args(**kwargs))[0]"},{"line_number":1774,"context_line":"        do_test(kwargs, [\u0027sda\u0027, \u0027sdb\u0027], [], True)"},{"line_number":1775,"context_line":"        do_test(worker_kwargs, [\u0027sda\u0027, \u0027sdb\u0027], [], True)"},{"line_number":1776,"context_line":""},{"line_number":1777,"context_line":"        # Have partition override, so no recon dump"}],"source_content_type":"text/x-python","patch_set":10,"id":"ff346bd7_a7b8ea29","line":1774,"range":{"start_line":1774,"start_character":44,"end_line":1774,"end_character":48},"updated":"2017-07-25 17:22:20.000000000","message":"this test isn\u0027t realistic - run_once would not be called with {\u0027devices\u0027: \u0027sda,sdb\u0027} if reconstructor_workers\u003d\u003d1, and the recon dump only occurs because reconstructor_workers\u003d\u003d1\n\nThe test should set reconstructor_workers\u003d0 when passing kwargs direct to run_once, which will make this expected value become False.","commit_id":"f7eb5af2b62d3a9dfe2b20c47da8dd2d5144326d"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"8b8a68cdf19bb7f8fd2bc604e0b1d82533f1de84","unresolved":false,"context_lines":[{"line_number":1772,"context_line":"        kwargs \u003d {\u0027devices\u0027: \u0027sda,sdb\u0027}"},{"line_number":1773,"context_line":"        worker_kwargs \u003d list(reconstructor.get_worker_args(**kwargs))[0]"},{"line_number":1774,"context_line":"        do_test(kwargs, [\u0027sda\u0027, \u0027sdb\u0027], [], True)"},{"line_number":1775,"context_line":"        do_test(worker_kwargs, [\u0027sda\u0027, \u0027sdb\u0027], [], True)"},{"line_number":1776,"context_line":""},{"line_number":1777,"context_line":"        # Have partition override, so no recon dump"},{"line_number":1778,"context_line":"        kwargs \u003d {\u0027partitions\u0027: \u00271,2,3\u0027}"}],"source_content_type":"text/x-python","patch_set":10,"id":"ff346bd7_f2c5928f","line":1775,"range":{"start_line":1775,"start_character":51,"end_line":1775,"end_character":55},"updated":"2017-07-25 17:22:20.000000000","message":"This seems like an anomalous result - run_once inline with an override device *does not* dump recon (legacy behaviour) but run_once in a worker with an override device *does* dump recon - I think that could be confusing.\n\nBut...if the recon stats that are dumped during run_once are *only* per disk stats then perhaps that is OK - if they are eventually aggregated along with *all* other source of per disk stats.","commit_id":"f7eb5af2b62d3a9dfe2b20c47da8dd2d5144326d"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"8b8a68cdf19bb7f8fd2bc604e0b1d82533f1de84","unresolved":false,"context_lines":[{"line_number":1785,"context_line":"        do_test(kwargs, [\u0027sda\u0027, \u0027sdb\u0027], [1, 2, 3], False)"},{"line_number":1786,"context_line":"        do_test(worker_kwargs, [\u0027sda\u0027, \u0027sdb\u0027], [1, 2, 3], False)"},{"line_number":1787,"context_line":""},{"line_number":1788,"context_line":"        # if the override device is not in local devices we still get"},{"line_number":1789,"context_line":"        # a recon dump, but it\u0027ll get cleaned up in the next aggregation"},{"line_number":1790,"context_line":"        kwargs \u003d {\u0027devices\u0027: \u0027sdz\u0027}"},{"line_number":1791,"context_line":"        worker_kwargs \u003d list(reconstructor.get_worker_args(**kwargs))[0]"},{"line_number":1792,"context_line":"        do_test(kwargs, [\u0027sdz\u0027], [], True)"}],"source_content_type":"text/x-python","patch_set":10,"id":"ff346bd7_53a57d03","line":1789,"range":{"start_line":1788,"start_character":8,"end_line":1789,"end_character":72},"updated":"2017-07-25 17:22:20.000000000","message":"that\u0027s unfortunate - can\u0027t we filter for known local devices in final_recon_dump?","commit_id":"f7eb5af2b62d3a9dfe2b20c47da8dd2d5144326d"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"8b8a68cdf19bb7f8fd2bc604e0b1d82533f1de84","unresolved":false,"context_lines":[{"line_number":1974,"context_line":"        self.assertEqual(set([\u0027d0\u0027, \u0027d1\u0027, \u0027d2\u0027, \u0027d3\u0027]),"},{"line_number":1975,"context_line":"                         set(data[\u0027object_reconstruction_per_disk\u0027].keys()))"},{"line_number":1976,"context_line":""},{"line_number":1977,"context_line":"        # it\u0027s idempotent"},{"line_number":1978,"context_line":"        reconstructor.aggregate_recon_update()"},{"line_number":1979,"context_line":"        with open(self.rcache) as f:"},{"line_number":1980,"context_line":"            data \u003d json.load(f)"}],"source_content_type":"text/x-python","patch_set":10,"id":"ff346bd7_ffc0ba7e","line":1977,"range":{"start_line":1977,"start_character":8,"end_line":1977,"end_character":25},"updated":"2017-07-25 17:22:20.000000000","message":"we\u0027ve no longer got the aggregates at this point, and those are the items for which we fixed idempotency...I\u0027ll split into two tests - one for waiting on all devices, one for removing non-local devices","commit_id":"f7eb5af2b62d3a9dfe2b20c47da8dd2d5144326d"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"c8ee9dd63063a8c5f6aed8c95b04b7de0e7ebdbe","unresolved":false,"context_lines":[{"line_number":1595,"context_line":"        reconstructor \u003d object_reconstructor.ObjectReconstructor("},{"line_number":1596,"context_line":"            {\u0027recon_cache_path\u0027: self.recon_cache_path},"},{"line_number":1597,"context_line":"            logger\u003dself.logger)"},{"line_number":1598,"context_line":"        reconstructor.all_local_devices \u003d lambda: [\u0027sda\u0027, \u0027sdc\u0027]"},{"line_number":1599,"context_line":"        total \u003d 12.0"},{"line_number":1600,"context_line":"        now \u003d time.time()"},{"line_number":1601,"context_line":"        with mock.patch(\u0027swift.obj.reconstructor.time.time\u0027, return_value\u003dnow):"}],"source_content_type":"text/x-python","patch_set":11,"id":"ff346bd7_0671eea9","line":1598,"updated":"2017-07-25 17:33:29.000000000","message":"for this test I made the override devices be in local devices","commit_id":"8e98ef43fb6f212f80ba37a0e22e3239f5d162ee"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"2a7b696199f166f6d857416527c60f867a68c3d8","unresolved":false,"context_lines":[{"line_number":1597,"context_line":"        # and empty"},{"line_number":1598,"context_line":"        self.assertEqual({}, data)"},{"line_number":1599,"context_line":""},{"line_number":1600,"context_line":"    def test_is_healthy_ring_update_next_check(self):"},{"line_number":1601,"context_line":"        reconstructor \u003d object_reconstructor.ObjectReconstructor("},{"line_number":1602,"context_line":"            {\u0027recon_cache_path\u0027: self.recon_cache_path},"},{"line_number":1603,"context_line":"            logger\u003dself.logger)"}],"source_content_type":"text/x-python","patch_set":13,"id":"ff346bd7_39596c34","line":1600,"range":{"start_line":1600,"start_character":29,"end_line":1600,"end_character":46},"updated":"2017-07-27 14:39:09.000000000","message":"nit: method name has become a little stale, perhaps just\n\n  test_is_healthy","commit_id":"701a172afac37229b85ea762f20428f6f422d29b"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"2a7b696199f166f6d857416527c60f867a68c3d8","unresolved":false,"context_lines":[{"line_number":1602,"context_line":"            {\u0027recon_cache_path\u0027: self.recon_cache_path},"},{"line_number":1603,"context_line":"            logger\u003dself.logger)"},{"line_number":1604,"context_line":"        self.assertTrue(reconstructor.is_healthy())"},{"line_number":1605,"context_line":"        reconstructor.get_local_devices \u003d lambda: {"},{"line_number":1606,"context_line":"            \u0027sdb%d\u0027 % p for p in reconstructor.policies}"},{"line_number":1607,"context_line":"        self.assertFalse(reconstructor.is_healthy())"},{"line_number":1608,"context_line":"        reconstructor.all_local_devices \u003d {"}],"source_content_type":"text/x-python","patch_set":13,"id":"ff346bd7_996178ac","line":1605,"range":{"start_line":1605,"start_character":22,"end_line":1605,"end_character":39},"updated":"2017-07-27 14:39:09.000000000","message":"nit: we don\u0027t have any test coverage for get_local_devices - for example if it were changed to cache all_local_devices","commit_id":"701a172afac37229b85ea762f20428f6f422d29b"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"50c795bf53bf94a91f22c9a6f9551be6812955ce","unresolved":false,"context_lines":[{"line_number":2027,"context_line":"        self.assertEqual(set([\u0027d0\u0027, \u0027d1\u0027, \u0027d2\u0027, \u0027d3\u0027]),"},{"line_number":2028,"context_line":"                         set(data[\u0027object_reconstruction_per_disk\u0027].keys()))"},{"line_number":2029,"context_line":""},{"line_number":2030,"context_line":"    def test_recon_aggregation_removes_devices(self):"},{"line_number":2031,"context_line":"        reconstructor \u003d object_reconstructor.ObjectReconstructor({"},{"line_number":2032,"context_line":"            \u0027reconstructor_workers\u0027: 2,"},{"line_number":2033,"context_line":"            \u0027recon_cache_path\u0027: self.recon_cache_path"}],"source_content_type":"text/x-python","patch_set":13,"id":"ff346bd7_bfb31598","line":2030,"updated":"2017-07-26 23:59:42.000000000","message":"Not so sure this test reflects reality any more... might should just drop it...","commit_id":"701a172afac37229b85ea762f20428f6f422d29b"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"2a7b696199f166f6d857416527c60f867a68c3d8","unresolved":false,"context_lines":[{"line_number":2027,"context_line":"        self.assertEqual(set([\u0027d0\u0027, \u0027d1\u0027, \u0027d2\u0027, \u0027d3\u0027]),"},{"line_number":2028,"context_line":"                         set(data[\u0027object_reconstruction_per_disk\u0027].keys()))"},{"line_number":2029,"context_line":""},{"line_number":2030,"context_line":"    def test_recon_aggregation_removes_devices(self):"},{"line_number":2031,"context_line":"        reconstructor \u003d object_reconstructor.ObjectReconstructor({"},{"line_number":2032,"context_line":"            \u0027reconstructor_workers\u0027: 2,"},{"line_number":2033,"context_line":"            \u0027recon_cache_path\u0027: self.recon_cache_path"}],"source_content_type":"text/x-python","patch_set":13,"id":"ff346bd7_79349493","line":2030,"in_reply_to":"ff346bd7_bfb31598","updated":"2017-07-27 14:39:09.000000000","message":"if I break aggregate_recon_update to not clear non-local devices then this is the only test to fail","commit_id":"701a172afac37229b85ea762f20428f6f422d29b"}]}