)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":7166,"name":"Sylvain Bauza","email":"sbauza@redhat.com","username":"sbauza"},"change_message_id":"fa6d49098cd16bd39f020b161c222a2655689de8","unresolved":false,"context_lines":[{"line_number":11,"context_line":"  enginefacade.transaction_context if it is reloaded"},{"line_number":12,"context_line":"  into the same process by a wsgi server and initalised"},{"line_number":13,"context_line":"  again after a failure."},{"line_number":14,"context_line":""},{"line_number":15,"context_line":"Change-Id: I704196711d30c1124e713ac31111a8ea6fa2f1ba"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"3f79a3b5_43ce892c","line":14,"updated":"2018-10-12 13:37:37.000000000","message":"need a bug report","commit_id":"1ae13fabe3b44b0547c3700d6474b43e4bb41f51"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"b767042af53a246a7be6d72fc146c4875c271b85","unresolved":false,"context_lines":[{"line_number":10,"context_line":"  to enable it to be called multiple times."},{"line_number":11,"context_line":"- If the transation_context is already configured"},{"line_number":12,"context_line":"  or started then a debug message will be emitted"},{"line_number":13,"context_line":"  and reconfiguration will be skipped."},{"line_number":14,"context_line":""},{"line_number":15,"context_line":"Closes-Bug: #1799246"},{"line_number":16,"context_line":"Related-Bug: #1784155"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":5,"id":"3f79a3b5_1790d295","line":13,"updated":"2018-10-24 22:14:46.000000000","message":"It would be nice if this commit message described what the bug is. I\u0027ve opened the Closes-Bug link twice now to remember what the problem was, what action did the user do to expose the bug, what error occurred because of the action. It would be helpful to be able to read it here every time I return to this review.","commit_id":"09243636fb55731f37fe44ad8ab440f17d3b466c"}],"nova/api/openstack/placement/db_api.py":[{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"8f4871be339f3670b747a94f224a75cc0e264346","unresolved":false,"context_lines":[{"line_number":17,"context_line":"from oslo_db.sqlalchemy import enginefacade"},{"line_number":18,"context_line":""},{"line_number":19,"context_line":""},{"line_number":20,"context_line":"placement_context_manager \u003d enginefacade.transaction_context()"},{"line_number":21,"context_line":""},{"line_number":22,"context_line":""},{"line_number":23,"context_line":"def _get_db_conf(conf_group):"}],"source_content_type":"text/x-python","patch_set":1,"id":"3f79a3b5_0bf811bf","line":20,"range":{"start_line":20,"start_character":1,"end_line":20,"end_character":62},"updated":"2018-10-19 17:08:24.000000000","message":"i spoke to matt booth about this offline and he suggested moving the initialization of this to the configure method such that if the wsgi app is reloaded we recreate the transaction context instead of skipping configuration.\n\nthis will have two side effects. \n\nfirst the current transaction context will continue to exist for as long a any operation that is currently using it holds a reference. when the reference count reaches zero it will be cleaned up by the python garbage collector.\n\nsecond if the configuration has been modified since the wsgi application first started by recreating the transaction context in configure we can ensure the future transaction will use the latest configuration.","commit_id":"1ae13fabe3b44b0547c3700d6474b43e4bb41f51"},{"author":{"_account_id":11564,"name":"Chris Dent","email":"cdent@anticdent.org","username":"chdent"},"change_message_id":"a78b086d5fd929ba4a4868afc57dfe14eedd908b","unresolved":false,"context_lines":[{"line_number":17,"context_line":"from oslo_db.sqlalchemy import enginefacade"},{"line_number":18,"context_line":""},{"line_number":19,"context_line":""},{"line_number":20,"context_line":"placement_context_manager \u003d enginefacade.transaction_context()"},{"line_number":21,"context_line":""},{"line_number":22,"context_line":""},{"line_number":23,"context_line":"def _get_db_conf(conf_group):"}],"source_content_type":"text/x-python","patch_set":1,"id":"3f79a3b5_31677a55","line":20,"range":{"start_line":20,"start_character":1,"end_line":20,"end_character":62},"in_reply_to":"3f79a3b5_0bf811bf","updated":"2018-10-19 19:22:24.000000000","message":"If we did move it within configure we\u0027d need to keep a module level reference because this symbol is used all over the place. Setting to None here and then resetting it in configure would probably work fine and have the third side effect of making it really obvious when configure wasn\u0027t getting called when it should be.","commit_id":"1ae13fabe3b44b0547c3700d6474b43e4bb41f51"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"d4d5fe9856c0ea36a7ac99580314617bc4c00a6d","unresolved":false,"context_lines":[{"line_number":17,"context_line":"from oslo_db.sqlalchemy import enginefacade"},{"line_number":18,"context_line":""},{"line_number":19,"context_line":""},{"line_number":20,"context_line":"placement_context_manager \u003d enginefacade.transaction_context()"},{"line_number":21,"context_line":""},{"line_number":22,"context_line":""},{"line_number":23,"context_line":"def _get_db_conf(conf_group):"}],"source_content_type":"text/x-python","patch_set":1,"id":"3f79a3b5_dfcf1a52","line":20,"range":{"start_line":20,"start_character":1,"end_line":20,"end_character":62},"in_reply_to":"3f79a3b5_31677a55","updated":"2018-10-22 11:32:55.000000000","message":"Chris, this was exactly my suggestion when Sean and I discussed it. Setting it to None here would also have the advantage of throwing an obvious error if we ever tried to use it before configure() was called.","commit_id":"1ae13fabe3b44b0547c3700d6474b43e4bb41f51"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"cd5d801860232c26a78d4e0ad535c3962d6a88b9","unresolved":false,"context_lines":[{"line_number":17,"context_line":"from oslo_db.sqlalchemy import enginefacade"},{"line_number":18,"context_line":""},{"line_number":19,"context_line":""},{"line_number":20,"context_line":"placement_context_manager \u003d enginefacade.transaction_context()"},{"line_number":21,"context_line":""},{"line_number":22,"context_line":""},{"line_number":23,"context_line":"def _get_db_conf(conf_group):"}],"source_content_type":"text/x-python","patch_set":1,"id":"3f79a3b5_c083baa9","line":20,"range":{"start_line":20,"start_character":1,"end_line":20,"end_character":62},"in_reply_to":"3f79a3b5_c0fe5a38","updated":"2018-10-22 13:14:45.000000000","message":"node we also use the decorator for read contexts statically \nhttps://github.com/openstack/nova/search?q\u003d%40db_api.placement_context_manager\u0026unscoped_q\u003d%40db_api.placement_context_manager","commit_id":"1ae13fabe3b44b0547c3700d6474b43e4bb41f51"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"9a4b89ed4ef36afd051e882b6884540738db2cdb","unresolved":false,"context_lines":[{"line_number":17,"context_line":"from oslo_db.sqlalchemy import enginefacade"},{"line_number":18,"context_line":""},{"line_number":19,"context_line":""},{"line_number":20,"context_line":"placement_context_manager \u003d enginefacade.transaction_context()"},{"line_number":21,"context_line":""},{"line_number":22,"context_line":""},{"line_number":23,"context_line":"def _get_db_conf(conf_group):"}],"source_content_type":"text/x-python","patch_set":1,"id":"3f79a3b5_c0fe5a38","line":20,"range":{"start_line":20,"start_character":1,"end_line":20,"end_character":62},"in_reply_to":"3f79a3b5_dfcf1a52","updated":"2018-10-22 13:12:32.000000000","message":"so setting this to none did highlight that we do use this before calling init indiretly\n\nwe retrive the writer context via a decorator \n@db_api.placement_context_manager.writer\nin nova/api/openstack/placement/objects/project.py line 27.\n\nthe decorators will not be recreated if we reassign the module level variable and the write context will keep the original transaction context alive.\n\nso we either also stop using the write decorators or we keep this form where we catch the exception and don\u0027t reconfigure.","commit_id":"1ae13fabe3b44b0547c3700d6474b43e4bb41f51"},{"author":{"_account_id":7166,"name":"Sylvain Bauza","email":"sbauza@redhat.com","username":"sbauza"},"change_message_id":"fa6d49098cd16bd39f020b161c222a2655689de8","unresolved":false,"context_lines":[{"line_number":34,"context_line":"        else:"},{"line_number":35,"context_line":"            placement_context_manager.configure("},{"line_number":36,"context_line":"                **_get_db_conf(conf.placement_database))"},{"line_number":37,"context_line":"    except TypeError:"},{"line_number":38,"context_line":"        # if placement is run in a wsgi server and is relaoded in the"},{"line_number":39,"context_line":"        # same process space after any trasaction has previously been"},{"line_number":40,"context_line":"        # created then we cannot reconfigure the transaction manager."}],"source_content_type":"text/x-python","patch_set":1,"id":"3f79a3b5_c344d9a1","line":37,"updated":"2018-10-12 13:37:37.000000000","message":"That\u0027s a bit difficult to understand why we only need to look at a TypeError.\n\nShould we maybe just look at an Exception ?","commit_id":"1ae13fabe3b44b0547c3700d6474b43e4bb41f51"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"c2dcc7d8da703ebda9137ecd2971c3a6adc55963","unresolved":false,"context_lines":[{"line_number":34,"context_line":"        else:"},{"line_number":35,"context_line":"            placement_context_manager.configure("},{"line_number":36,"context_line":"                **_get_db_conf(conf.placement_database))"},{"line_number":37,"context_line":"    except TypeError:"},{"line_number":38,"context_line":"        # if placement is run in a wsgi server and is relaoded in the"},{"line_number":39,"context_line":"        # same process space after any trasaction has previously been"},{"line_number":40,"context_line":"        # created then we cannot reconfigure the transaction manager."}],"source_content_type":"text/x-python","patch_set":1,"id":"3f79a3b5_e37235e4","line":37,"in_reply_to":"3f79a3b5_c344d9a1","updated":"2018-10-12 13:49:51.000000000","message":"we only look for type error because of \nhttps://github.com/openstack/oslo.db/blob/master/oslo_db/sqlalchemy/enginefacade.py#L316-L317","commit_id":"1ae13fabe3b44b0547c3700d6474b43e4bb41f51"},{"author":{"_account_id":11564,"name":"Chris Dent","email":"cdent@anticdent.org","username":"chdent"},"change_message_id":"a78b086d5fd929ba4a4868afc57dfe14eedd908b","unresolved":false,"context_lines":[{"line_number":34,"context_line":"        else:"},{"line_number":35,"context_line":"            placement_context_manager.configure("},{"line_number":36,"context_line":"                **_get_db_conf(conf.placement_database))"},{"line_number":37,"context_line":"    except TypeError:"},{"line_number":38,"context_line":"        # if placement is run in a wsgi server and is relaoded in the"},{"line_number":39,"context_line":"        # same process space after any trasaction has previously been"},{"line_number":40,"context_line":"        # created then we cannot reconfigure the transaction manager."}],"source_content_type":"text/x-python","patch_set":1,"id":"3f79a3b5_3139ba3f","line":37,"in_reply_to":"3f79a3b5_e37235e4","updated":"2018-10-19 19:22:24.000000000","message":"I agree that this should be as limited as possible in terms of which exception it traps. Catching Exception is _always_ a bad idea, especially if you think you\u0027re doing it because you want to be sure to catch everything. You never want to catch everything.","commit_id":"1ae13fabe3b44b0547c3700d6474b43e4bb41f51"},{"author":{"_account_id":7166,"name":"Sylvain Bauza","email":"sbauza@redhat.com","username":"sbauza"},"change_message_id":"fa6d49098cd16bd39f020b161c222a2655689de8","unresolved":false,"context_lines":[{"line_number":37,"context_line":"    except TypeError:"},{"line_number":38,"context_line":"        # if placement is run in a wsgi server and is relaoded in the"},{"line_number":39,"context_line":"        # same process space after any trasaction has previously been"},{"line_number":40,"context_line":"        # created then we cannot reconfigure the transaction manager."},{"line_number":41,"context_line":"        pass"},{"line_number":42,"context_line":""},{"line_number":43,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"3f79a3b5_83c82121","line":40,"updated":"2018-10-12 13:37:37.000000000","message":"we need to explain more about the problem.","commit_id":"1ae13fabe3b44b0547c3700d6474b43e4bb41f51"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"c2dcc7d8da703ebda9137ecd2971c3a6adc55963","unresolved":false,"context_lines":[{"line_number":37,"context_line":"    except TypeError:"},{"line_number":38,"context_line":"        # if placement is run in a wsgi server and is relaoded in the"},{"line_number":39,"context_line":"        # same process space after any trasaction has previously been"},{"line_number":40,"context_line":"        # created then we cannot reconfigure the transaction manager."},{"line_number":41,"context_line":"        pass"},{"line_number":42,"context_line":""},{"line_number":43,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"3f79a3b5_3ea282fc","line":40,"in_reply_to":"3f79a3b5_83c82121","updated":"2018-10-12 13:49:51.000000000","message":"yes but i would need more info from lee to write a better message \n\nthe only info i have to go on is this error message\n\nhttps://bugzilla.redhat.com/show_bug.cgi?id\u003d1630069#c23\n\nwhich show we are raising the type error","commit_id":"1ae13fabe3b44b0547c3700d6474b43e4bb41f51"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"d6cc8c348fbe218fe2224ce2bf36f0274f14f739","unresolved":false,"context_lines":[{"line_number":34,"context_line":"        else:"},{"line_number":35,"context_line":"            placement_context_manager.configure("},{"line_number":36,"context_line":"                **_get_db_conf(conf.placement_database))"},{"line_number":37,"context_line":"    except TypeError:"},{"line_number":38,"context_line":"        # NOTE(sean-k-mooney): We only catch TypeError as this is the"},{"line_number":39,"context_line":"        # exception raised if a transation_context is started"},{"line_number":40,"context_line":"        # when configure is called. If the placement api is run via a"}],"source_content_type":"text/x-python","patch_set":3,"id":"3f79a3b5_0739c1ef","line":37,"range":{"start_line":37,"start_character":4,"end_line":37,"end_character":21},"updated":"2018-10-22 18:24:18.000000000","message":"For reviewers: here\u0027s where TypeError is raised from configure in oslo.db:\n\nhttps://github.com/openstack/oslo.db/blob/dff11337f55b7a9a28d7c30658df51b7b0b443eb/oslo_db/sqlalchemy/enginefacade.py#L317","commit_id":"dc8ccf28885eea4df3dbbf77dfeb21b27defc22a"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"d6cc8c348fbe218fe2224ce2bf36f0274f14f739","unresolved":false,"context_lines":[{"line_number":41,"context_line":"        # mod_wsgi and the wsgi application is relaoded, it will be executed"},{"line_number":42,"context_line":"        # in the same process space. As such after any trasaction has"},{"line_number":43,"context_line":"        # previously been started we cannot reconfigure the transaction"},{"line_number":44,"context_line":"        # context manager without raising a TypeError."},{"line_number":45,"context_line":"        pass"},{"line_number":46,"context_line":""},{"line_number":47,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"3f79a3b5_8a87185c","line":44,"updated":"2018-10-22 18:24:18.000000000","message":"Unfortunately, this paragraph didn\u0027t clearly convey to me what the problem is. Maybe it\u0027s just me, but this would make a lot more sense if it instead said something like:\n\n\"We catch and ignore TypeError because oslo.db raises TypeError if configure is called after the TransactionFactory has already been started. If the placement api is run via mod_wsgi and the wsgi application is reloaded, it will be executed in the same process space. In this case, we will call configure during the reload and oslo.db will raise TypeError because reconfiguration is not needed.\"","commit_id":"dc8ccf28885eea4df3dbbf77dfeb21b27defc22a"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"bc7cba6b33aebb92f7059a232c1d3e489b153378","unresolved":false,"context_lines":[{"line_number":41,"context_line":"        # mod_wsgi and the wsgi application is relaoded, it will be executed"},{"line_number":42,"context_line":"        # in the same process space. As such after any trasaction has"},{"line_number":43,"context_line":"        # previously been started we cannot reconfigure the transaction"},{"line_number":44,"context_line":"        # context manager without raising a TypeError."},{"line_number":45,"context_line":"        pass"},{"line_number":46,"context_line":""},{"line_number":47,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"3f79a3b5_6ce1742b","line":44,"in_reply_to":"3f79a3b5_8a87185c","updated":"2018-10-22 23:54:04.000000000","message":"thanks that is concise.\ni will rephrase as you suggest.","commit_id":"dc8ccf28885eea4df3dbbf77dfeb21b27defc22a"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"dc5f7a0db5c4086f6b5dc55daf07199942b29cc0","unresolved":false,"context_lines":[{"line_number":18,"context_line":"from oslo_log import log as logging"},{"line_number":19,"context_line":""},{"line_number":20,"context_line":""},{"line_number":21,"context_line":"_configured \u003d False"},{"line_number":22,"context_line":"placement_context_manager \u003d enginefacade.transaction_context()"},{"line_number":23,"context_line":"LOG \u003d logging.getLogger(__name__)"},{"line_number":24,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"3f79a3b5_8be9251a","line":21,"range":{"start_line":21,"start_character":0,"end_line":21,"end_character":19},"updated":"2018-10-24 18:51:23.000000000","message":"i have added configured so this can be eaisilly backported.","commit_id":"09243636fb55731f37fe44ad8ab440f17d3b466c"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"dc5f7a0db5c4086f6b5dc55daf07199942b29cc0","unresolved":false,"context_lines":[{"line_number":29,"context_line":""},{"line_number":30,"context_line":"def configure(conf):"},{"line_number":31,"context_line":"    ctx \u003d placement_context_manager"},{"line_number":32,"context_line":"    started \u003d getattr(ctx, \"is_started\", False) or ctx._factory._started"},{"line_number":33,"context_line":"    global _configured"},{"line_number":34,"context_line":"    if _configured or started:"},{"line_number":35,"context_line":"        LOG.debug(\"Transaction context already started, not reconfiguring.\")"}],"source_content_type":"text/x-python","patch_set":5,"id":"3f79a3b5_6b5b69f8","line":32,"range":{"start_line":32,"start_character":4,"end_line":32,"end_character":72},"updated":"2018-10-24 18:51:23.000000000","message":"but started is the actual requirement.\n\ni can just remove this is we want but i kindof feel the exception version was nicer.\n\nif people agree however that the simpelset fix e.g. just the _configured flag is the most desireable ill go with that\neven though it will skip configuring in some case it technically could.","commit_id":"09243636fb55731f37fe44ad8ab440f17d3b466c"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"b9a50fd54341cd4242542cc835fe175c755c7fca","unresolved":false,"context_lines":[{"line_number":31,"context_line":"    ctx \u003d placement_context_manager"},{"line_number":32,"context_line":"    started \u003d getattr(ctx, \"is_started\", False) or ctx._factory._started"},{"line_number":33,"context_line":"    global _configured"},{"line_number":34,"context_line":"    if _configured or started:"},{"line_number":35,"context_line":"        LOG.debug(\"Transaction context already started, not reconfiguring.\")"},{"line_number":36,"context_line":"    else:"},{"line_number":37,"context_line":"        # If [placement_database]/connection is not set in conf, then placement"}],"source_content_type":"text/x-python","patch_set":5,"id":"3f79a3b5_eb3699e8","line":34,"updated":"2018-10-24 18:38:58.000000000","message":"Why do we need to check for started at all? That\u0027s a private attribute in an oslo.db object. Isn\u0027t it enough to just make sure we don\u0027t call configure twice?","commit_id":"09243636fb55731f37fe44ad8ab440f17d3b466c"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"e5b0e7e96db8435a1c0ed2d9750c8b3f139df63e","unresolved":false,"context_lines":[{"line_number":31,"context_line":"    ctx \u003d placement_context_manager"},{"line_number":32,"context_line":"    started \u003d getattr(ctx, \"is_started\", False) or ctx._factory._started"},{"line_number":33,"context_line":"    global _configured"},{"line_number":34,"context_line":"    if _configured or started:"},{"line_number":35,"context_line":"        LOG.debug(\"Transaction context already started, not reconfiguring.\")"},{"line_number":36,"context_line":"    else:"},{"line_number":37,"context_line":"        # If [placement_database]/connection is not set in conf, then placement"}],"source_content_type":"text/x-python","patch_set":5,"id":"3f79a3b5_d1b34a34","line":34,"in_reply_to":"3f79a3b5_4be6adb1","updated":"2018-10-24 20:24:12.000000000","message":"this is actully a better reference for what is happening \nhttps://modwsgi.readthedocs.io/en/develop/user-guides/processes-and-threading.html#python-sub-interpreters\n\nas well as \n\nhttps://modwsgi.readthedocs.io/en/develop/user-guides/reloading-source-code.html?highlight\u003ddaemon%20mode#reloading-in-daemon-mode\n\n\ntripleo appears to be useing more or less the defautl wsgi config in deamon mode\nhttps://github.com/rdo-packages/nova-distgit/blob/rpm-master/nova-placement-api.conf#L5-L7\n\nbased on \nhttps://modwsgi.readthedocs.io/en/develop/user-guides/processes-and-threading.html#python-sub-interpreters\nthe key points are A.) \n\"When a sub interpreter is created for a WSGI application, it would then normally persist for the life of the process\"\nand \nB.) by setting the WSGIApplicationGroup %{GLOBAL} \"each WSGI application will then have access to the same global data.\"\n\n\nthis is global/module level data so it will get loaded once and stay loaded for the life time of the wsgi process even if the application is restrated in the interpreter.\n\nwe are the ortinial code violated teh requirement in pont3 of https://modwsgi.readthedocs.io/en/develop/user-guides/processes-and-threading.html#building-a-portable-application\n\nit is not re-entrant.","commit_id":"09243636fb55731f37fe44ad8ab440f17d3b466c"},{"author":{"_account_id":11816,"name":"mike bayer","display_name":"Mike Bayer","email":"mike_mp@zzzcomputing.com","username":"zzzeek","status":"Red Hat"},"change_message_id":"d4d990eda59ace1d70487b971aaf836ec21f556a","unresolved":false,"context_lines":[{"line_number":31,"context_line":"    ctx \u003d placement_context_manager"},{"line_number":32,"context_line":"    started \u003d getattr(ctx, \"is_started\", False) or ctx._factory._started"},{"line_number":33,"context_line":"    global _configured"},{"line_number":34,"context_line":"    if _configured or started:"},{"line_number":35,"context_line":"        LOG.debug(\"Transaction context already started, not reconfiguring.\")"},{"line_number":36,"context_line":"    else:"},{"line_number":37,"context_line":"        # If [placement_database]/connection is not set in conf, then placement"}],"source_content_type":"text/x-python","patch_set":5,"id":"3f79a3b5_77b7c6f9","line":34,"in_reply_to":"3f79a3b5_5c6991f4","updated":"2018-10-24 22:34:16.000000000","message":"OK, but the code as written, _configured and started will never be different, and if you were to set _configured from the outside (public API would be needed for this to be useful since you don\u0027t want code accessing _configured from the outside), then the code will fail.  Because if started is True, you *cannot* call context_manager.configure() again it will raise.  enginefacade does not yet have a reconfigure-in-place option, and I\u0027d like to add one as it\u0027s not hard to do, but this is a new feature and I don\u0027t think qualifies for backporting.\n\nSo again in the spirit of the minimal change for the time being, given that \"reconfigure\" is not an option, a simple \"run_once\" decorator on top would suffice for now.","commit_id":"09243636fb55731f37fe44ad8ab440f17d3b466c"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"b767042af53a246a7be6d72fc146c4875c271b85","unresolved":false,"context_lines":[{"line_number":31,"context_line":"    ctx \u003d placement_context_manager"},{"line_number":32,"context_line":"    started \u003d getattr(ctx, \"is_started\", False) or ctx._factory._started"},{"line_number":33,"context_line":"    global _configured"},{"line_number":34,"context_line":"    if _configured or started:"},{"line_number":35,"context_line":"        LOG.debug(\"Transaction context already started, not reconfiguring.\")"},{"line_number":36,"context_line":"    else:"},{"line_number":37,"context_line":"        # If [placement_database]/connection is not set in conf, then placement"}],"source_content_type":"text/x-python","patch_set":5,"id":"3f79a3b5_379ceebc","line":34,"in_reply_to":"3f79a3b5_5c6991f4","updated":"2018-10-24 22:14:46.000000000","message":"This is a good point. In general, services support handling of SIGHUP to trigger a config reload without a service restart. If this change would prevent that from being possible, then that\u0027s a concern.","commit_id":"09243636fb55731f37fe44ad8ab440f17d3b466c"},{"author":{"_account_id":11564,"name":"Chris Dent","email":"cdent@anticdent.org","username":"chdent"},"change_message_id":"a6d0e9e8f248b1195b3baf79c98cc4421d6f110c","unresolved":false,"context_lines":[{"line_number":31,"context_line":"    ctx \u003d placement_context_manager"},{"line_number":32,"context_line":"    started \u003d getattr(ctx, \"is_started\", False) or ctx._factory._started"},{"line_number":33,"context_line":"    global _configured"},{"line_number":34,"context_line":"    if _configured or started:"},{"line_number":35,"context_line":"        LOG.debug(\"Transaction context already started, not reconfiguring.\")"},{"line_number":36,"context_line":"    else:"},{"line_number":37,"context_line":"        # If [placement_database]/connection is not set in conf, then placement"}],"source_content_type":"text/x-python","patch_set":5,"id":"3f79a3b5_5c6991f4","line":34,"in_reply_to":"3f79a3b5_5c9bd114","updated":"2018-10-24 21:59:01.000000000","message":"The issue with that is that sometimes you _do_ want reconfigure, especially if you send a hub to the wsgidaemonprocess, or things of that ilk. Causing a clean slate (for oslo conf and enginefacade) in init_application ought to (may?) allow that.","commit_id":"09243636fb55731f37fe44ad8ab440f17d3b466c"},{"author":{"_account_id":11816,"name":"mike bayer","display_name":"Mike Bayer","email":"mike_mp@zzzcomputing.com","username":"zzzeek","status":"Red Hat"},"change_message_id":"f6c9c9d52241ca23b7e295494ce4c36c47d994fa","unresolved":false,"context_lines":[{"line_number":31,"context_line":"    ctx \u003d placement_context_manager"},{"line_number":32,"context_line":"    started \u003d getattr(ctx, \"is_started\", False) or ctx._factory._started"},{"line_number":33,"context_line":"    global _configured"},{"line_number":34,"context_line":"    if _configured or started:"},{"line_number":35,"context_line":"        LOG.debug(\"Transaction context already started, not reconfiguring.\")"},{"line_number":36,"context_line":"    else:"},{"line_number":37,"context_line":"        # If [placement_database]/connection is not set in conf, then placement"}],"source_content_type":"text/x-python","patch_set":5,"id":"3f79a3b5_5c9bd114","line":34,"in_reply_to":"3f79a3b5_9c2ba9e8","updated":"2018-10-24 21:55:01.000000000","message":"why is there both \"_configured\" and \"_started\" and when would these be different?\n\nnow that I\u0027m fully awake it seems like this would look a lot cleaner if we just had a simple decoartor in utils:\n\n\tdef call_once(fn):\n\t    _called \u003d [False]\n\t    def go(*arg, **kw):\n\t        if not _called[0]:\n\t            try:\n\t                fn(*arg, **kw)\n\t            finally:\n\t                 _called[0] \u003d True\n\t    return go\n\nthen in this file, just need a one line change:\n\n    @call_once\n    def configure(conf):\n        # ....\n\nthat\u0027s assuming the first time you get \"conf\", that\u0027s the only time you need to look at it.","commit_id":"09243636fb55731f37fe44ad8ab440f17d3b466c"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"08e9c2c9376efa206103f78d2d26cb03ebe943a2","unresolved":false,"context_lines":[{"line_number":31,"context_line":"    ctx \u003d placement_context_manager"},{"line_number":32,"context_line":"    started \u003d getattr(ctx, \"is_started\", False) or ctx._factory._started"},{"line_number":33,"context_line":"    global _configured"},{"line_number":34,"context_line":"    if _configured or started:"},{"line_number":35,"context_line":"        LOG.debug(\"Transaction context already started, not reconfiguring.\")"},{"line_number":36,"context_line":"    else:"},{"line_number":37,"context_line":"        # If [placement_database]/connection is not set in conf, then placement"}],"source_content_type":"text/x-python","patch_set":5,"id":"3f79a3b5_4be6adb1","line":34,"in_reply_to":"3f79a3b5_abd181fb","updated":"2018-10-24 19:20:16.000000000","message":"yes we know what mod_wsgi does is dump \n\nhttps://modwsgi.readthedocs.io/en/develop/user-guides/application-issues.html#reloading-python-interpreters\n\nbut we cant change the fact that kolla and tripplo both\nrun placment under mod_wsgi in the stable branches.\n\ngoing forward we could swap to uwsgi but since this was\nconsiderd a autimation and release blocker for osp14\nhttps://bugzilla.redhat.com/show_bug.cgi?id\u003d1630069\n\na minimal fix to nova/placement seams more achievable.\n\ntriple o are also trying to work around this on there side to not cause the db race that uncovered this on upgrade also\nso we are trying to fix this in multiple ways without boiling the ocean of change acpache\u0027s mod_wsgi or replaceing it with uwsgi during the osp 14 beta.\n\nthe tripleo bug is here https://bugs.launchpad.net/tripleo/+bug/1784155","commit_id":"09243636fb55731f37fe44ad8ab440f17d3b466c"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"ef72c70b3f6b53665c795d6440b03d45f7162914","unresolved":false,"context_lines":[{"line_number":31,"context_line":"    ctx \u003d placement_context_manager"},{"line_number":32,"context_line":"    started \u003d getattr(ctx, \"is_started\", False) or ctx._factory._started"},{"line_number":33,"context_line":"    global _configured"},{"line_number":34,"context_line":"    if _configured or started:"},{"line_number":35,"context_line":"        LOG.debug(\"Transaction context already started, not reconfiguring.\")"},{"line_number":36,"context_line":"    else:"},{"line_number":37,"context_line":"        # If [placement_database]/connection is not set in conf, then placement"}],"source_content_type":"text/x-python","patch_set":5,"id":"3f79a3b5_fc6e5d95","line":34,"in_reply_to":"3f79a3b5_b13fae21","updated":"2018-10-24 21:25:16.000000000","message":"ok yes i do like that more but it is also a larger chagne.\n\nit will proable take a little bit to make sure we have reset everythin that needs to be reset but i should be doable.\n\nthis fix may have just uncoverd another issue anyway.\n\nim not sure full re-entrancy is want we want but it is what we need based on the mod_wsgi docs as far as i can tell so as long as we consider that to be a supprot configurtion we likely shoudl be trying to get to full re-entrancy.\n\nwe are not using python threading, multi processs or eventlets in placement luckily so that makes re-entrancy simpler as there is no concurancy within the  wsgi hosting process that we need to deal with.","commit_id":"09243636fb55731f37fe44ad8ab440f17d3b466c"},{"author":{"_account_id":11816,"name":"mike bayer","display_name":"Mike Bayer","email":"mike_mp@zzzcomputing.com","username":"zzzeek","status":"Red Hat"},"change_message_id":"5f10fac886e9738fbf21627de039fa184a72c49a","unresolved":false,"context_lines":[{"line_number":31,"context_line":"    ctx \u003d placement_context_manager"},{"line_number":32,"context_line":"    started \u003d getattr(ctx, \"is_started\", False) or ctx._factory._started"},{"line_number":33,"context_line":"    global _configured"},{"line_number":34,"context_line":"    if _configured or started:"},{"line_number":35,"context_line":"        LOG.debug(\"Transaction context already started, not reconfiguring.\")"},{"line_number":36,"context_line":"    else:"},{"line_number":37,"context_line":"        # If [placement_database]/connection is not set in conf, then placement"}],"source_content_type":"text/x-python","patch_set":5,"id":"3f79a3b5_abd181fb","line":34,"in_reply_to":"3f79a3b5_cba07d91","updated":"2018-10-24 19:06:49.000000000","message":"this doesn\u0027t seem right.  it\u0027s reusing the interpreter, and all of it\u0027s in-memory state, but it is somehow causing the application to re-initialize itself?  why would it do the latter if not the former?   This seems like it would be causing all kinds of problems all over the place.   If the whole interpreter stays up, it shouldn\u0027t be calling any kind of startup routines either.   this is really where this issue should be fixed.","commit_id":"09243636fb55731f37fe44ad8ab440f17d3b466c"},{"author":{"_account_id":11564,"name":"Chris Dent","email":"cdent@anticdent.org","username":"chdent"},"change_message_id":"00ae28abe909f31ddef47cbf674e4c3ce0720320","unresolved":false,"context_lines":[{"line_number":31,"context_line":"    ctx \u003d placement_context_manager"},{"line_number":32,"context_line":"    started \u003d getattr(ctx, \"is_started\", False) or ctx._factory._started"},{"line_number":33,"context_line":"    global _configured"},{"line_number":34,"context_line":"    if _configured or started:"},{"line_number":35,"context_line":"        LOG.debug(\"Transaction context already started, not reconfiguring.\")"},{"line_number":36,"context_line":"    else:"},{"line_number":37,"context_line":"        # If [placement_database]/connection is not set in conf, then placement"}],"source_content_type":"text/x-python","patch_set":5,"id":"3f79a3b5_b13fae21","line":34,"in_reply_to":"3f79a3b5_d1b34a34","updated":"2018-10-24 20:55:08.000000000","message":"\u003e it is not re-entrant.\n\nIf that\u0027s what we truly want/need then the changes we want shouldn\u0027t happen in this module, but at https://github.com/openstack/placement/blob/master/placement/wsgi.py#L101 (and the equivalent in the nova copy of placement) akin the conf.reset() that lee added a while back. We want to reset the conf and dispose of any enginefacades where the application is \"created\".\n\nHowever, note that neither of the idiomatic (i.e. globals) ways of using oslo_config and oslo_db are really going to be happy with this if we simultaneously want to be supporting config reloads and things we\u0027re calling \"restarts\" but are more akin to a reload.\n\nI think we need to unwind the several different things we\u0027re trying to fix here and enumerate them a little more effectively than merely having a chat in gerrit or IRC because it seems like we get a few minutes into each discussion and start conflating the issues.\n\nAs placement is currently written it assume that both of the global-like things we are talking about here (config and enginefacade factory) are the same for every single request and do not change through the lifetime of the request. This is safe in any instance where we\u0027re not trying to call init_application a second time in the same process. Try this code to see it in action:\n\n    from placement import wsgi\n\n    # oslo config will spew a lot of output but errors\n    app \u003d wsgi.init_application()\n\n    # traceback\n    app2 \u003d wsgi.init_application()\n\nBefore Lee\u0027s patch that would have barfed on conf, now it barfs on the transaction factory.\n\nSo what we should do it, in wsgi.py in something like a _refresh method, not in this, set things up so that when db_api.configure is called, what it is called with is _clean_. That is, if the placement_context_manager is started at the start of init_application, we should stop it, much like conf is reset.","commit_id":"09243636fb55731f37fe44ad8ab440f17d3b466c"},{"author":{"_account_id":11816,"name":"mike bayer","display_name":"Mike Bayer","email":"mike_mp@zzzcomputing.com","username":"zzzeek","status":"Red Hat"},"change_message_id":"ca2f141db6db47fab038882f00fed73aebc72b8f","unresolved":false,"context_lines":[{"line_number":31,"context_line":"    ctx \u003d placement_context_manager"},{"line_number":32,"context_line":"    started \u003d getattr(ctx, \"is_started\", False) or ctx._factory._started"},{"line_number":33,"context_line":"    global _configured"},{"line_number":34,"context_line":"    if _configured or started:"},{"line_number":35,"context_line":"        LOG.debug(\"Transaction context already started, not reconfiguring.\")"},{"line_number":36,"context_line":"    else:"},{"line_number":37,"context_line":"        # If [placement_database]/connection is not set in conf, then placement"}],"source_content_type":"text/x-python","patch_set":5,"id":"3f79a3b5_ebc0d97f","line":34,"in_reply_to":"3f79a3b5_eb3699e8","updated":"2018-10-24 18:49:17.000000000","message":"\"is_started\" is a public attribute being added in https://review.openstack.org/#/c/613040/.  _started is for backwards compatibility.   you need to check for started because enginefacade cannot operate with this:\n\n    facade.configure(some_parameter\u003d\u0027foo\u0027)\n    engine \u003d facade.get_engine()   \n    facade.configure(some_parameter\u003d\u0027bar\u0027) # \u003c-- impossible, the engine is already generated\n\n\nnow, looking at the above, I realize *another* way to do this would be to call a \"stop()\" method on the facade itself that calls dispose_pool() (to discard engines) and also resets the _started flag and all of its conditions.    But bigger issue, I\u0027m not exactly sure what the wsgi pattern in use is here such that the same module is still in memory bu the configure() step is being called again.\n\nCan this configure() step be receiving a new conf that has a totally different database URL and other options?  If so, then this approach should be changed such that the existing placement_context_manager is internally disposed so that it can establish the new connectivity.    dispose_pool() does this partially but additional methods for clearing out the \"started\" state would need to be added as well.","commit_id":"09243636fb55731f37fe44ad8ab440f17d3b466c"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"dc5f7a0db5c4086f6b5dc55daf07199942b29cc0","unresolved":false,"context_lines":[{"line_number":31,"context_line":"    ctx \u003d placement_context_manager"},{"line_number":32,"context_line":"    started \u003d getattr(ctx, \"is_started\", False) or ctx._factory._started"},{"line_number":33,"context_line":"    global _configured"},{"line_number":34,"context_line":"    if _configured or started:"},{"line_number":35,"context_line":"        LOG.debug(\"Transaction context already started, not reconfiguring.\")"},{"line_number":36,"context_line":"    else:"},{"line_number":37,"context_line":"        # If [placement_database]/connection is not set in conf, then placement"}],"source_content_type":"text/x-python","patch_set":5,"id":"3f79a3b5_2b757186","line":34,"in_reply_to":"3f79a3b5_eb3699e8","updated":"2018-10-24 18:51:23.000000000","message":"this is future proofing for https://review.openstack.org/#/c/613040/2 which is making it public so we can use it to check. \n\ntechnically its safe to reconfigure as long as its\nnot started so without the not started check there\nare some cases where we could legally reconfigure that we don\u0027t.","commit_id":"09243636fb55731f37fe44ad8ab440f17d3b466c"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"2874fb58b837f3060c4fe554ac7d9685f22486a1","unresolved":false,"context_lines":[{"line_number":31,"context_line":"    ctx \u003d placement_context_manager"},{"line_number":32,"context_line":"    started \u003d getattr(ctx, \"is_started\", False) or ctx._factory._started"},{"line_number":33,"context_line":"    global _configured"},{"line_number":34,"context_line":"    if _configured or started:"},{"line_number":35,"context_line":"        LOG.debug(\"Transaction context already started, not reconfiguring.\")"},{"line_number":36,"context_line":"    else:"},{"line_number":37,"context_line":"        # If [placement_database]/connection is not set in conf, then placement"}],"source_content_type":"text/x-python","patch_set":5,"id":"3f79a3b5_cba07d91","line":34,"in_reply_to":"3f79a3b5_ebc0d97f","updated":"2018-10-24 19:02:59.000000000","message":"by the way the useage pattern as far as i understand it is that if placement crash for some reason e.g. it loses connection to the db because we are dong a db sync then the wsgi app will exit but mod_wsgi will restart the application when the next api request is recived reusing the same interperter.\n\nso any module level variables will have a lifetime that our lives the lifetime of the wsgi application which in my view should be able to assume that when the enrty point of the wsgi script si executed that it is in a clean interpreter.\n\nmod_wsgi reuses the interpreter to avoid the overhead of potentially recreating it for every api request for wsgi application that exit after serving a single request.\n\nthat reuse break our mental model when programming as we do not expect gloal or module level variable to outlive the program and this is the root of this issue. it is not how uwsgi works which is how we test wsgi services in the gate.","commit_id":"09243636fb55731f37fe44ad8ab440f17d3b466c"},{"author":{"_account_id":11564,"name":"Chris Dent","email":"cdent@anticdent.org","username":"chdent"},"change_message_id":"35d3452a771d8f62c414d6b1e8c993877756273f","unresolved":false,"context_lines":[{"line_number":31,"context_line":"    ctx \u003d placement_context_manager"},{"line_number":32,"context_line":"    started \u003d getattr(ctx, \"is_started\", False) or ctx._factory._started"},{"line_number":33,"context_line":"    global _configured"},{"line_number":34,"context_line":"    if _configured or started:"},{"line_number":35,"context_line":"        LOG.debug(\"Transaction context already started, not reconfiguring.\")"},{"line_number":36,"context_line":"    else:"},{"line_number":37,"context_line":"        # If [placement_database]/connection is not set in conf, then placement"}],"source_content_type":"text/x-python","patch_set":5,"id":"3f79a3b5_9c2ba9e8","line":34,"in_reply_to":"3f79a3b5_fc6e5d95","updated":"2018-10-24 21:41:40.000000000","message":"I don\u0027t think you should try to address everything in one go. Add un-starting the transaction factory somewhere in init-application and just that.\n\nThere shouldn\u0027t be much more than that actually matters: because of the way wsgi and webob work once you are inside the handler code things should be a lot more clean.","commit_id":"09243636fb55731f37fe44ad8ab440f17d3b466c"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"b9a50fd54341cd4242542cc835fe175c755c7fca","unresolved":false,"context_lines":[{"line_number":32,"context_line":"    started \u003d getattr(ctx, \"is_started\", False) or ctx._factory._started"},{"line_number":33,"context_line":"    global _configured"},{"line_number":34,"context_line":"    if _configured or started:"},{"line_number":35,"context_line":"        LOG.debug(\"Transaction context already started, not reconfiguring.\")"},{"line_number":36,"context_line":"    else:"},{"line_number":37,"context_line":"        # If [placement_database]/connection is not set in conf, then placement"},{"line_number":38,"context_line":"        # data will be stored in the nova_api database."}],"source_content_type":"text/x-python","patch_set":5,"id":"3f79a3b5_c8fba313","line":35,"range":{"start_line":35,"start_character":19,"end_line":35,"end_character":38},"updated":"2018-10-24 18:38:58.000000000","message":"TransactionFactory","commit_id":"09243636fb55731f37fe44ad8ab440f17d3b466c"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"e9481c0328ca87559d51237fafe66ad8077bc127","unresolved":false,"context_lines":[{"line_number":26,"context_line":"    return dict(conf_group.items())"},{"line_number":27,"context_line":""},{"line_number":28,"context_line":""},{"line_number":29,"context_line":"@run_once(\"TransactionFactory already started, not reconfiguring.\", LOG.debug)"},{"line_number":30,"context_line":"def configure(conf):"},{"line_number":31,"context_line":"    # If [placement_database]/connection is not set in conf, then placement"},{"line_number":32,"context_line":"    # data will be stored in the nova_api database."}],"source_content_type":"text/x-python","patch_set":6,"id":"3f79a3b5_c494bc37","line":29,"range":{"start_line":29,"start_character":72,"end_line":29,"end_character":77},"updated":"2018-10-26 16:27:48.000000000","message":"Bikeshed: info, warning even?","commit_id":"a13ecf680e4414fd788b46afc528e552a468cafc"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"ae487eff5e5bacea9379ac48ab65dee7ed90bbde","unresolved":false,"context_lines":[{"line_number":26,"context_line":"    return dict(conf_group.items())"},{"line_number":27,"context_line":""},{"line_number":28,"context_line":""},{"line_number":29,"context_line":"@run_once(\"TransactionFactory already started, not reconfiguring.\", LOG.debug)"},{"line_number":30,"context_line":"def configure(conf):"},{"line_number":31,"context_line":"    # If [placement_database]/connection is not set in conf, then placement"},{"line_number":32,"context_line":"    # data will be stored in the nova_api database."}],"source_content_type":"text/x-python","patch_set":6,"id":"3f79a3b5_da8a3ebb","line":29,"range":{"start_line":29,"start_character":72,"end_line":29,"end_character":77},"in_reply_to":"3f79a3b5_929da2bb","updated":"2018-10-29 11:07:11.000000000","message":"It might conceivably affect what they were trying to do, because it didn\u0027t reload the configuration they changed? Just thinking it might prevent a head-scratcher.","commit_id":"a13ecf680e4414fd788b46afc528e552a468cafc"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"8aaae770497552835328b82f688ed7f7fb4ad53d","unresolved":false,"context_lines":[{"line_number":26,"context_line":"    return dict(conf_group.items())"},{"line_number":27,"context_line":""},{"line_number":28,"context_line":""},{"line_number":29,"context_line":"@run_once(\"TransactionFactory already started, not reconfiguring.\", LOG.debug)"},{"line_number":30,"context_line":"def configure(conf):"},{"line_number":31,"context_line":"    # If [placement_database]/connection is not set in conf, then placement"},{"line_number":32,"context_line":"    # data will be stored in the nova_api database."}],"source_content_type":"text/x-python","patch_set":6,"id":"3f79a3b5_929da2bb","line":29,"range":{"start_line":29,"start_character":72,"end_line":29,"end_character":77},"in_reply_to":"3f79a3b5_c494bc37","updated":"2018-10-26 17:33:56.000000000","message":"there is nothing a operator can really do to fix this so that is why i chose debug.\n\ni mean they could restart the apache2 instance but thats not going to supress this longterm so there is no real reason to emit a warning. if people really want a warning i can respin but i think it would just add noise.","commit_id":"a13ecf680e4414fd788b46afc528e552a468cafc"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"35308ef14d7457f5f5f95e35f278e6315b2d4391","unresolved":false,"context_lines":[{"line_number":26,"context_line":"    return dict(conf_group.items())"},{"line_number":27,"context_line":""},{"line_number":28,"context_line":""},{"line_number":29,"context_line":"@run_once(\"TransactionFactory already started, not reconfiguring.\", LOG.debug)"},{"line_number":30,"context_line":"def configure(conf):"},{"line_number":31,"context_line":"    # If [placement_database]/connection is not set in conf, then placement"},{"line_number":32,"context_line":"    # data will be stored in the nova_api database."}],"source_content_type":"text/x-python","patch_set":6,"id":"3f79a3b5_d4619cd7","line":29,"range":{"start_line":29,"start_character":72,"end_line":29,"end_character":77},"in_reply_to":"3f79a3b5_da8a3ebb","updated":"2018-10-30 11:08:44.000000000","message":"ok i need to respin anyway and that is a reasonable argument so ill change this to warning.","commit_id":"a13ecf680e4414fd788b46afc528e552a468cafc"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"45cd1c36a59890518ba62e59a4157f0d28740648","unresolved":false,"context_lines":[{"line_number":26,"context_line":"    return dict(conf_group.items())"},{"line_number":27,"context_line":""},{"line_number":28,"context_line":""},{"line_number":29,"context_line":"@run_once(\"TransactionFactory already started, not reconfiguring.\", LOG.warning)"},{"line_number":30,"context_line":"def configure(conf):"},{"line_number":31,"context_line":"    # If [placement_database]/connection is not set in conf, then placement"},{"line_number":32,"context_line":"    # data will be stored in the nova_api database."}],"source_content_type":"text/x-python","patch_set":9,"id":"3f79a3b5_db3d442a","line":29,"range":{"start_line":29,"start_character":0,"end_line":29,"end_character":80},"updated":"2018-10-30 16:46:08.000000000","message":"the flake8 test for 79 charthers instead of 80 so this is 1 column too wide :(","commit_id":"85fe614edfebfc027d82e4435a5d6ca764a76c63"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"60eb9fe84f4e04c92c0812ae09d7d31237ba7936","unresolved":false,"context_lines":[{"line_number":26,"context_line":"    return dict(conf_group.items())"},{"line_number":27,"context_line":""},{"line_number":28,"context_line":""},{"line_number":29,"context_line":"@run_once(\"TransactionFactory already started, not reconfiguring.\","},{"line_number":30,"context_line":"          LOG.warning)"},{"line_number":31,"context_line":"def configure(conf):"},{"line_number":32,"context_line":"    # If [placement_database]/connection is not set in conf, then placement"},{"line_number":33,"context_line":"    # data will be stored in the nova_api database."},{"line_number":34,"context_line":"    if conf.placement_database.connection is None:"}],"source_content_type":"text/x-python","patch_set":10,"id":"3f79a3b5_fc0aefcd","line":31,"range":{"start_line":29,"start_character":0,"end_line":31,"end_character":20},"updated":"2018-11-06 15:24:55.000000000","message":"I did some background thinking about this. We can only run this once because somewhere deep in the call chain there is process global state. The @run_once decorator needs to introduce another global state (the configure.called flag  (( it is global as configure function object is global)) to guard this call.\n\nSo configure() is already, indirectly, depending on a global state deep down in the call chain. Can we simply say?:\n\ndef configure(conf):\n    if not \u003cglobal state is set\u003e:\n        placement_context_manager.configure(\n            **_get_db_conf(conf.api_database))\n\n\n\nwould\u0027nt it become simply this?:\n\ndef configure(conf):\n    if not placement_db.placement_context_manager._factory._started:\n        placement_context_manager.configure(\n            **_get_db_conf(conf.api_database))","commit_id":"711b16b60a42beb4a9b9ea9d5f4ab02a4cadc77f"}],"nova/tests/unit/api/openstack/placement/test_db_api.py":[{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"5783e80dd254e5b59cde25514a9500250136de01","unresolved":false,"context_lines":[{"line_number":1,"context_line":"# All Rights Reserved."},{"line_number":2,"context_line":"#"},{"line_number":3,"context_line":"#    Licensed under the Apache License, Version 2.0 (the \"License\"); you may"},{"line_number":4,"context_line":"#    not use this file except in compliance with the License. You may obtain"}],"source_content_type":"text/x-python","patch_set":1,"id":"3f79a3b5_4332095f","line":1,"range":{"start_line":1,"start_character":0,"end_line":1,"end_character":22},"updated":"2018-10-12 13:33:20.000000000","message":"Missing copyright?","commit_id":"1ae13fabe3b44b0547c3700d6474b43e4bb41f51"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"c2dcc7d8da703ebda9137ecd2971c3a6adc55963","unresolved":false,"context_lines":[{"line_number":1,"context_line":"# All Rights Reserved."},{"line_number":2,"context_line":"#"},{"line_number":3,"context_line":"#    Licensed under the Apache License, Version 2.0 (the \"License\"); you may"},{"line_number":4,"context_line":"#    not use this file except in compliance with the License. You may obtain"}],"source_content_type":"text/x-python","patch_set":1,"id":"3f79a3b5_e320f5b3","line":1,"range":{"start_line":1,"start_character":0,"end_line":1,"end_character":22},"in_reply_to":"3f79a3b5_4332095f","updated":"2018-10-12 13:49:51.000000000","message":"i copied this from another file i generally do not add copyright notices just the license.","commit_id":"1ae13fabe3b44b0547c3700d6474b43e4bb41f51"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"5783e80dd254e5b59cde25514a9500250136de01","unresolved":false,"context_lines":[{"line_number":11,"context_line":"#    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the"},{"line_number":12,"context_line":"#    License for the specific language governing permissions and limitations"},{"line_number":13,"context_line":"#    under the License."},{"line_number":14,"context_line":""},{"line_number":15,"context_line":""},{"line_number":16,"context_line":"import mock"},{"line_number":17,"context_line":"import testtools"}],"source_content_type":"text/x-python","patch_set":1,"id":"3f79a3b5_a340bda4","line":14,"updated":"2018-10-12 13:33:20.000000000","message":"nit: extra newlines","commit_id":"1ae13fabe3b44b0547c3700d6474b43e4bb41f51"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"c2dcc7d8da703ebda9137ecd2971c3a6adc55963","unresolved":false,"context_lines":[{"line_number":11,"context_line":"#    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the"},{"line_number":12,"context_line":"#    License for the specific language governing permissions and limitations"},{"line_number":13,"context_line":"#    under the License."},{"line_number":14,"context_line":""},{"line_number":15,"context_line":""},{"line_number":16,"context_line":"import mock"},{"line_number":17,"context_line":"import testtools"}],"source_content_type":"text/x-python","patch_set":1,"id":"3f79a3b5_1e01c603","line":14,"in_reply_to":"3f79a3b5_a340bda4","updated":"2018-10-12 13:49:51.000000000","message":"i think pep8 need the two line here.\nill check","commit_id":"1ae13fabe3b44b0547c3700d6474b43e4bb41f51"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"5783e80dd254e5b59cde25514a9500250136de01","unresolved":false,"context_lines":[{"line_number":15,"context_line":""},{"line_number":16,"context_line":"import mock"},{"line_number":17,"context_line":"import testtools"},{"line_number":18,"context_line":""},{"line_number":19,"context_line":"from oslo_config import cfg"},{"line_number":20,"context_line":""},{"line_number":21,"context_line":"from nova.api.openstack.placement import db_api"}],"source_content_type":"text/x-python","patch_set":1,"id":"3f79a3b5_631065ac","line":18,"updated":"2018-10-12 13:33:20.000000000","message":"nit: here too","commit_id":"1ae13fabe3b44b0547c3700d6474b43e4bb41f51"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"c2dcc7d8da703ebda9137ecd2971c3a6adc55963","unresolved":false,"context_lines":[{"line_number":15,"context_line":""},{"line_number":16,"context_line":"import mock"},{"line_number":17,"context_line":"import testtools"},{"line_number":18,"context_line":""},{"line_number":19,"context_line":"from oslo_config import cfg"},{"line_number":20,"context_line":""},{"line_number":21,"context_line":"from nova.api.openstack.placement import db_api"}],"source_content_type":"text/x-python","patch_set":1,"id":"3f79a3b5_c3fcd90e","line":18,"in_reply_to":"3f79a3b5_631065ac","updated":"2018-10-12 13:49:51.000000000","message":"this is here intentionally\n\ni always group by\n\nimport\n\nfrom other_project import x\n\nfrom this project import x","commit_id":"1ae13fabe3b44b0547c3700d6474b43e4bb41f51"},{"author":{"_account_id":11564,"name":"Chris Dent","email":"cdent@anticdent.org","username":"chdent"},"change_message_id":"a78b086d5fd929ba4a4868afc57dfe14eedd908b","unresolved":false,"context_lines":[{"line_number":33,"context_line":""},{"line_number":34,"context_line":"    @mock.patch.object(db_api.placement_context_manager._factory,"},{"line_number":35,"context_line":"                       \"_started\", True)"},{"line_number":36,"context_line":"    def test_calling_configre_on_started_context_raises(self):"},{"line_number":37,"context_line":"        self.assertRaises(TypeError, db_api.placement_context_manager.configure"},{"line_number":38,"context_line":"                          , CONF.api_database)"},{"line_number":39,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"3f79a3b5_514b16b1","line":36,"range":{"start_line":36,"start_character":22,"end_line":36,"end_character":29},"updated":"2018-10-19 19:22:24.000000000","message":"configure","commit_id":"1ae13fabe3b44b0547c3700d6474b43e4bb41f51"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"5783e80dd254e5b59cde25514a9500250136de01","unresolved":false,"context_lines":[{"line_number":34,"context_line":"    @mock.patch.object(db_api.placement_context_manager._factory,"},{"line_number":35,"context_line":"                       \"_started\", True)"},{"line_number":36,"context_line":"    def test_calling_configre_on_started_context_raises(self):"},{"line_number":37,"context_line":"        self.assertRaises(TypeError, db_api.placement_context_manager.configure"},{"line_number":38,"context_line":"                          , CONF.api_database)"},{"line_number":39,"context_line":""},{"line_number":40,"context_line":""},{"line_number":41,"context_line":"                "}],"source_content_type":"text/x-python","patch_set":1,"id":"3f79a3b5_63270520","line":41,"range":{"start_line":37,"start_character":0,"end_line":41,"end_character":16},"updated":"2018-10-12 13:33:20.000000000","message":"Oops","commit_id":"1ae13fabe3b44b0547c3700d6474b43e4bb41f51"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"c2dcc7d8da703ebda9137ecd2971c3a6adc55963","unresolved":false,"context_lines":[{"line_number":34,"context_line":"    @mock.patch.object(db_api.placement_context_manager._factory,"},{"line_number":35,"context_line":"                       \"_started\", True)"},{"line_number":36,"context_line":"    def test_calling_configre_on_started_context_raises(self):"},{"line_number":37,"context_line":"        self.assertRaises(TypeError, db_api.placement_context_manager.configure"},{"line_number":38,"context_line":"                          , CONF.api_database)"},{"line_number":39,"context_line":""},{"line_number":40,"context_line":""},{"line_number":41,"context_line":"                "}],"source_content_type":"text/x-python","patch_set":1,"id":"3f79a3b5_c32379ba","line":41,"range":{"start_line":37,"start_character":0,"end_line":41,"end_character":16},"in_reply_to":"3f79a3b5_63270520","updated":"2018-10-12 13:49:51.000000000","message":"hum this passes pep8 locally i will fix the extra line.","commit_id":"1ae13fabe3b44b0547c3700d6474b43e4bb41f51"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"bc7cba6b33aebb92f7059a232c1d3e489b153378","unresolved":false,"context_lines":[{"line_number":31,"context_line":""},{"line_number":32,"context_line":"    @mock.patch.object(db_api, \"placement_context_manager\", autospec\u003dTrue)"},{"line_number":33,"context_line":"    def test_can_call_configure_twice(self, ctx):"},{"line_number":34,"context_line":"        db_api.configure(self.conf_fixture.conf)"},{"line_number":35,"context_line":"        ctx.configure.side_effect \u003d TypeError()"},{"line_number":36,"context_line":"        db_api.configure(self.conf_fixture.conf)"},{"line_number":37,"context_line":""},{"line_number":38,"context_line":"    @mock.patch.object(db_api.placement_context_manager._factory,"},{"line_number":39,"context_line":"                       \"_started\", True)"}],"source_content_type":"text/x-python","patch_set":3,"id":"3f79a3b5_8ce65013","line":36,"range":{"start_line":34,"start_character":8,"end_line":36,"end_character":48},"updated":"2018-10-22 23:54:04.000000000","message":"it should be noted that if configure did not catch the TypeError internally then this would fail however this\ndoes not test that calling configure on a started context\nnolonger fails which is the purpose of the second test below.","commit_id":"dc8ccf28885eea4df3dbbf77dfeb21b27defc22a"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"d6cc8c348fbe218fe2224ce2bf36f0274f14f739","unresolved":false,"context_lines":[{"line_number":35,"context_line":"        ctx.configure.side_effect \u003d TypeError()"},{"line_number":36,"context_line":"        db_api.configure(self.conf_fixture.conf)"},{"line_number":37,"context_line":""},{"line_number":38,"context_line":"    @mock.patch.object(db_api.placement_context_manager._factory,"},{"line_number":39,"context_line":"                       \"_started\", True)"},{"line_number":40,"context_line":"    def test_calling_configure_on_started_context_raises(self):"},{"line_number":41,"context_line":"        self.assertRaises(TypeError,"},{"line_number":42,"context_line":"            db_api.placement_context_manager.configure,"},{"line_number":43,"context_line":"            self.conf_fixture.conf.placement_database)"}],"source_content_type":"text/x-python","patch_set":3,"id":"3f79a3b5_87a1513d","line":43,"range":{"start_line":38,"start_character":4,"end_line":43,"end_character":54},"updated":"2018-10-22 18:24:18.000000000","message":"This test seems to really be testing oslo.db itself. Is the concern being able to catch if oslo.db ever changes the behavior for the error when a context is already started?","commit_id":"dc8ccf28885eea4df3dbbf77dfeb21b27defc22a"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"335dff99be71f7b0d9a1f05d9b9958b06ee2fad6","unresolved":false,"context_lines":[{"line_number":35,"context_line":"        ctx.configure.side_effect \u003d TypeError()"},{"line_number":36,"context_line":"        db_api.configure(self.conf_fixture.conf)"},{"line_number":37,"context_line":""},{"line_number":38,"context_line":"    @mock.patch.object(db_api.placement_context_manager._factory,"},{"line_number":39,"context_line":"                       \"_started\", True)"},{"line_number":40,"context_line":"    def test_calling_configure_on_started_context_raises(self):"},{"line_number":41,"context_line":"        self.assertRaises(TypeError,"},{"line_number":42,"context_line":"            db_api.placement_context_manager.configure,"},{"line_number":43,"context_line":"            self.conf_fixture.conf.placement_database)"}],"source_content_type":"text/x-python","patch_set":3,"id":"3f79a3b5_0aca289f","line":43,"range":{"start_line":38,"start_character":4,"end_line":43,"end_character":54},"in_reply_to":"3f79a3b5_87a1513d","updated":"2018-10-22 18:25:19.000000000","message":"s/context/TransactionFactory/","commit_id":"dc8ccf28885eea4df3dbbf77dfeb21b27defc22a"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"bc7cba6b33aebb92f7059a232c1d3e489b153378","unresolved":false,"context_lines":[{"line_number":35,"context_line":"        ctx.configure.side_effect \u003d TypeError()"},{"line_number":36,"context_line":"        db_api.configure(self.conf_fixture.conf)"},{"line_number":37,"context_line":""},{"line_number":38,"context_line":"    @mock.patch.object(db_api.placement_context_manager._factory,"},{"line_number":39,"context_line":"                       \"_started\", True)"},{"line_number":40,"context_line":"    def test_calling_configure_on_started_context_raises(self):"},{"line_number":41,"context_line":"        self.assertRaises(TypeError,"},{"line_number":42,"context_line":"            db_api.placement_context_manager.configure,"},{"line_number":43,"context_line":"            self.conf_fixture.conf.placement_database)"}],"source_content_type":"text/x-python","patch_set":3,"id":"3f79a3b5_ecd7c421","line":43,"range":{"start_line":38,"start_character":4,"end_line":43,"end_character":54},"in_reply_to":"3f79a3b5_87a1513d","updated":"2018-10-22 23:54:04.000000000","message":"this was partially a sanity check.\n\ninitially i wanted to test i could repoduce the observed error from the donwstream bug in a miniumal test case so this is effectively a reproducer/regression test for the observed behavior of the bug.\n\nif oslo.db was ever to change the type of the exception raised or nolonger raise an exception this would catch that change in behavior and atleast highlihgt that we might need\nto alter our use of the transation_context but you are correct that this is touching more of the internal implementaiton of oslo.db then i would like.\n\ni can remove or alter this if you have a prefernce but im not sure of a better way to mock the behavior of invoking configure on a started transaction context.","commit_id":"dc8ccf28885eea4df3dbbf77dfeb21b27defc22a"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"b9a50fd54341cd4242542cc835fe175c755c7fca","unresolved":false,"context_lines":[{"line_number":57,"context_line":""},{"line_number":58,"context_line":"    @mock.patch.object(db_api.placement_context_manager._factory,"},{"line_number":59,"context_line":"                       \"_started\", True)"},{"line_number":60,"context_line":"    def test_calling_configure_on_started_transaction_factory_raises(self):"},{"line_number":61,"context_line":"        \"\"\"This test asserts the observed behaviour in bug #1799246."},{"line_number":62,"context_line":"        It is intended to catch any future changes in that behaviour which may"},{"line_number":63,"context_line":"        require changes to prevent a regression."}],"source_content_type":"text/x-python","patch_set":5,"id":"3f79a3b5_6b484961","line":60,"updated":"2018-10-24 18:38:58.000000000","message":"If we don\u0027t check for started, then we don\u0027t need this test.","commit_id":"09243636fb55731f37fe44ad8ab440f17d3b466c"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"e9481c0328ca87559d51237fafe66ad8077bc127","unresolved":false,"context_lines":[{"line_number":23,"context_line":"CONF \u003d cfg.CONF"},{"line_number":24,"context_line":""},{"line_number":25,"context_line":""},{"line_number":26,"context_line":"class DbApiTests(testtools.TestCase):"},{"line_number":27,"context_line":""},{"line_number":28,"context_line":"    def setUp(self):"},{"line_number":29,"context_line":"        super(DbApiTests, self).setUp()"}],"source_content_type":"text/x-python","patch_set":6,"id":"3f79a3b5_44b98cc2","line":26,"updated":"2018-10-26 16:27:48.000000000","message":"I\u0027d be inclined to rewrite these as tests for run_once. With that in place, I don\u0027t think we\u0027d necessarily test separately that this doesn\u0027t run twice. The latter sounds a bit more integration testy to me.","commit_id":"a13ecf680e4414fd788b46afc528e552a468cafc"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"8aaae770497552835328b82f688ed7f7fb4ad53d","unresolved":false,"context_lines":[{"line_number":23,"context_line":"CONF \u003d cfg.CONF"},{"line_number":24,"context_line":""},{"line_number":25,"context_line":""},{"line_number":26,"context_line":"class DbApiTests(testtools.TestCase):"},{"line_number":27,"context_line":""},{"line_number":28,"context_line":"    def setUp(self):"},{"line_number":29,"context_line":"        super(DbApiTests, self).setUp()"}],"source_content_type":"text/x-python","patch_set":6,"id":"3f79a3b5_f275f697","line":26,"in_reply_to":"3f79a3b5_44b98cc2","updated":"2018-10-26 17:33:56.000000000","message":"no these are defintely unit tests.\n\nintegration tests contain no mocks and require a fully running system. here i am testing the minimal code i can\nto assert the behavior i care about.\n\ni could add more tests for run_once but i really hate that this module had no unit test before so i dont want to leave it that way going forward.","commit_id":"a13ecf680e4414fd788b46afc528e552a468cafc"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"ae487eff5e5bacea9379ac48ab65dee7ed90bbde","unresolved":false,"context_lines":[{"line_number":23,"context_line":"CONF \u003d cfg.CONF"},{"line_number":24,"context_line":""},{"line_number":25,"context_line":""},{"line_number":26,"context_line":"class DbApiTests(testtools.TestCase):"},{"line_number":27,"context_line":""},{"line_number":28,"context_line":"    def setUp(self):"},{"line_number":29,"context_line":"        super(DbApiTests, self).setUp()"}],"source_content_type":"text/x-python","patch_set":6,"id":"3f79a3b5_faf05a62","line":26,"in_reply_to":"3f79a3b5_927cc222","updated":"2018-10-29 11:07:11.000000000","message":"run_once is a unit-testy kind of thing, and these would make decent unit tests for it.\n\nA good functional test for this bug, rather than that run_once works as expected, would be that we don\u0027t bork when initialising the wsgi application twice.","commit_id":"a13ecf680e4414fd788b46afc528e552a468cafc"},{"author":{"_account_id":11564,"name":"Chris Dent","email":"cdent@anticdent.org","username":"chdent"},"change_message_id":"3c1d1963edc2f57f176160b0bbdc4f30b906577f","unresolved":false,"context_lines":[{"line_number":23,"context_line":"CONF \u003d cfg.CONF"},{"line_number":24,"context_line":""},{"line_number":25,"context_line":""},{"line_number":26,"context_line":"class DbApiTests(testtools.TestCase):"},{"line_number":27,"context_line":""},{"line_number":28,"context_line":"    def setUp(self):"},{"line_number":29,"context_line":"        super(DbApiTests, self).setUp()"}],"source_content_type":"text/x-python","patch_set":6,"id":"3f79a3b5_927cc222","line":26,"in_reply_to":"3f79a3b5_f275f697","updated":"2018-10-26 17:46:25.000000000","message":"\u003e i could add more tests for run_once but i really hate that this\n \u003e module had no unit test before so i dont want to leave it that way\n \u003e going forward.\n\nI don\u0027t wish to discourage you from writing unit tests for placement-related stuff, but we\u0027ve got a history of orienting most tests towards functional since it is primarily web and db stuff that we want to test. And coverage ends up being relatively good: http://logs.openstack.org/08/607508/6/check/openstack-tox-cover/1d3537b/cover/\n\nNot that coverage means very much (if anything).\n\nThis change is clearly the sort of thing that wants a unit test.","commit_id":"a13ecf680e4414fd788b46afc528e552a468cafc"},{"author":{"_account_id":11564,"name":"Chris Dent","email":"cdent@anticdent.org","username":"chdent"},"change_message_id":"b996d853f7cc875b2431b9b3c97ac7e6d0462a90","unresolved":false,"context_lines":[{"line_number":23,"context_line":"CONF \u003d cfg.CONF"},{"line_number":24,"context_line":""},{"line_number":25,"context_line":""},{"line_number":26,"context_line":"class DbApiTests(testtools.TestCase):"},{"line_number":27,"context_line":""},{"line_number":28,"context_line":"    def setUp(self):"},{"line_number":29,"context_line":"        super(DbApiTests, self).setUp()"}],"source_content_type":"text/x-python","patch_set":6,"id":"3f79a3b5_081e290c","line":26,"in_reply_to":"3f79a3b5_faf05a62","updated":"2018-10-29 18:40:12.000000000","message":"\u003e run_once is a unit-testy kind of thing, and these would make decent\n \u003e unit tests for it.\n\nYeah, it would make sense for run_once itself to have it\u0027s own test in unit/test_utils.py\n\n \u003e A good functional test for this bug, rather than that run_once\n \u003e works as expected, would be that we don\u0027t bork when initialising\n \u003e the wsgi application twice.\n\nYes, that seems like a good plan.","commit_id":"a13ecf680e4414fd788b46afc528e552a468cafc"},{"author":{"_account_id":11564,"name":"Chris Dent","email":"cdent@anticdent.org","username":"chdent"},"change_message_id":"b996d853f7cc875b2431b9b3c97ac7e6d0462a90","unresolved":false,"context_lines":[{"line_number":35,"context_line":"        which may happen if placement is run under mod_wsgi and the"},{"line_number":36,"context_line":"        wsgi application is reloaded."},{"line_number":37,"context_line":"        \"\"\""},{"line_number":38,"context_line":"        self.assertFalse(db_api.configure.called)"},{"line_number":39,"context_line":"        db_api.configure(self.conf_fixture.conf)"},{"line_number":40,"context_line":"        configure_mock.assert_called_once()"},{"line_number":41,"context_line":"        self.assertTrue(db_api.configure.called)"}],"source_content_type":"text/x-python","patch_set":6,"id":"3f79a3b5_0852a968","line":38,"updated":"2018-10-29 18:40:12.000000000","message":"Based on http://logs.openstack.org/34/610034/6/check/openstack-tox-py27/92f57f2/testr_results.html.gz looks like this can fail, which makes sense because you\u0027re testing db_api.configure (the method in the module), not the mock (which db_api.configure calls).\n\nIn fact I\u0027m entirely confused. What\u0027s actually going on here? Can you call \u0027called\u0027 on a non mocked thing?\n\nIn any case this seems more complicated than what we really want, which is two things:\n\n* confirm that run_once does what it claims to do\n* confirm that calling the init_application method doesn\u0027t blow up a second time round","commit_id":"a13ecf680e4414fd788b46afc528e552a468cafc"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"35308ef14d7457f5f5f95e35f278e6315b2d4391","unresolved":false,"context_lines":[{"line_number":35,"context_line":"        which may happen if placement is run under mod_wsgi and the"},{"line_number":36,"context_line":"        wsgi application is reloaded."},{"line_number":37,"context_line":"        \"\"\""},{"line_number":38,"context_line":"        self.assertFalse(db_api.configure.called)"},{"line_number":39,"context_line":"        db_api.configure(self.conf_fixture.conf)"},{"line_number":40,"context_line":"        configure_mock.assert_called_once()"},{"line_number":41,"context_line":"        self.assertTrue(db_api.configure.called)"}],"source_content_type":"text/x-python","patch_set":6,"id":"3f79a3b5_f41618f4","line":38,"in_reply_to":"3f79a3b5_0852a968","updated":"2018-10-30 11:08:44.000000000","message":"the infureating thing is this is passing locally because of how i am running the test e.g. im just running test tests.\n\ncalled in this case is coming form the run_once decorator.\n\ndb_api.configure is the functor object returned by the run_once decorator so i was asserting that we had not called\nit before the test ran. however since i have not mocked it if the test ran interleave with other tests then this is nondeterministic as the other test may have ran already and called this method.\n\nso yes im going to test the run_once decorator seperatly and simply these tests.","commit_id":"a13ecf680e4414fd788b46afc528e552a468cafc"},{"author":{"_account_id":11564,"name":"Chris Dent","email":"cdent@anticdent.org","username":"chdent"},"change_message_id":"c50c087f8745b2c790e69d0ef6bfed79958e279e","unresolved":false,"context_lines":[{"line_number":31,"context_line":"        db_api.configure.reset()"},{"line_number":32,"context_line":""},{"line_number":33,"context_line":"    @mock.patch.object(db_api.placement_context_manager, \"configure\")"},{"line_number":34,"context_line":"    def test_can_call_configure_twice(self, configure_mock):"},{"line_number":35,"context_line":"        \"\"\"This test asserts that configure can be safely called twice"},{"line_number":36,"context_line":"        which may happen if placement is run under mod_wsgi and the"},{"line_number":37,"context_line":"        wsgi application is reloaded."}],"source_content_type":"text/x-python","patch_set":8,"id":"3f79a3b5_291af56a","line":34,"updated":"2018-10-30 15:49:32.000000000","message":"This test manages to make a lot more sense than the previous version, thanks for that.","commit_id":"d32fbc139ef25fc55e9e4530751a35cdde1d5928"},{"author":{"_account_id":11564,"name":"Chris Dent","email":"cdent@anticdent.org","username":"chdent"},"change_message_id":"c50c087f8745b2c790e69d0ef6bfed79958e279e","unresolved":false,"context_lines":[{"line_number":46,"context_line":""},{"line_number":47,"context_line":"        db_api.configure(self.conf_fixture.conf)"},{"line_number":48,"context_line":"        # Note we have not reset the mock so it should"},{"line_number":49,"context_line":"        # have been called once form the first invoation of"},{"line_number":50,"context_line":"        # db_api.configure and the second invocation should not"},{"line_number":51,"context_line":"        # have called it again"},{"line_number":52,"context_line":"        configure_mock.assert_called_once()"}],"source_content_type":"text/x-python","patch_set":8,"id":"3f79a3b5_892549b0","line":49,"range":{"start_line":49,"start_character":32,"end_line":49,"end_character":36},"updated":"2018-10-30 15:49:32.000000000","message":"from","commit_id":"d32fbc139ef25fc55e9e4530751a35cdde1d5928"}],"nova/tests/unit/test_utils.py":[{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"6b11a64de08c3972a32a3cdb981b7be7d759dc31","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":11,"id":"3f79a3b5_46a8c572","line":1365,"updated":"2018-11-08 10:00:40.000000000","message":"this is missing coverage for exception handling behavior and cleanup behavior. I\u0027ve proposed the test in a followup. https://review.openstack.org/#/c/616485","commit_id":"65610f01e8fa81a4c57615dbc02bf09dc4be6e68"}],"nova/utils.py":[{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"e9481c0328ca87559d51237fafe66ad8077bc127","unresolved":false,"context_lines":[{"line_number":1322,"context_line":"    def outter_wrapper(func):"},{"line_number":1323,"context_line":"        @functools.wraps(func)"},{"line_number":1324,"context_line":"        def wrapper(*args, **kwargs):"},{"line_number":1325,"context_line":"            if not wrapper.called:"},{"line_number":1326,"context_line":"                ret \u003d func(*args, **kwargs)"},{"line_number":1327,"context_line":"                wrapper.called \u003d True"},{"line_number":1328,"context_line":"                return ret"},{"line_number":1329,"context_line":"            else:"},{"line_number":1330,"context_line":"                logger(message)"},{"line_number":1331,"context_line":"        wrapper.called \u003d False"},{"line_number":1332,"context_line":""}],"source_content_type":"text/x-python","patch_set":6,"id":"3f79a3b5_44d8aceb","line":1329,"range":{"start_line":1325,"start_character":0,"end_line":1329,"end_character":1},"updated":"2018-10-26 16:27:48.000000000","message":"No error handling, but YAGNI.","commit_id":"a13ecf680e4414fd788b46afc528e552a468cafc"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"8aaae770497552835328b82f688ed7f7fb4ad53d","unresolved":false,"context_lines":[{"line_number":1322,"context_line":"    def outter_wrapper(func):"},{"line_number":1323,"context_line":"        @functools.wraps(func)"},{"line_number":1324,"context_line":"        def wrapper(*args, **kwargs):"},{"line_number":1325,"context_line":"            if not wrapper.called:"},{"line_number":1326,"context_line":"                ret \u003d func(*args, **kwargs)"},{"line_number":1327,"context_line":"                wrapper.called \u003d True"},{"line_number":1328,"context_line":"                return ret"},{"line_number":1329,"context_line":"            else:"},{"line_number":1330,"context_line":"                logger(message)"},{"line_number":1331,"context_line":"        wrapper.called \u003d False"},{"line_number":1332,"context_line":""}],"source_content_type":"text/x-python","patch_set":6,"id":"3f79a3b5_670a8a27","line":1329,"range":{"start_line":1325,"start_character":0,"end_line":1329,"end_character":1},"in_reply_to":"3f79a3b5_44d8aceb","updated":"2018-10-26 17:33:56.000000000","message":"yes this is intentionaly as i want any exceptions form the wrapped function to be uncaught. \n\nits also why i set called after i invoke the function not before so it will only be called if no exception is raised.\n\nthis is also not c so we dont signel failure but return value :)","commit_id":"a13ecf680e4414fd788b46afc528e552a468cafc"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"e9481c0328ca87559d51237fafe66ad8077bc127","unresolved":false,"context_lines":[{"line_number":1327,"context_line":"                wrapper.called \u003d True"},{"line_number":1328,"context_line":"                return ret"},{"line_number":1329,"context_line":"            else:"},{"line_number":1330,"context_line":"                logger(message)"},{"line_number":1331,"context_line":"        wrapper.called \u003d False"},{"line_number":1332,"context_line":""},{"line_number":1333,"context_line":"        def reset(wrapper, *args, **kwargs):"}],"source_content_type":"text/x-python","patch_set":6,"id":"3f79a3b5_c4ccbc34","line":1330,"range":{"start_line":1330,"start_character":1,"end_line":1330,"end_character":31},"updated":"2018-10-26 16:27:48.000000000","message":"This doesn\u0027t return anything, but don\u0027t bother because YAGNI.","commit_id":"a13ecf680e4414fd788b46afc528e552a468cafc"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"e9481c0328ca87559d51237fafe66ad8077bc127","unresolved":false,"context_lines":[{"line_number":1335,"context_line":"            if cleanup:"},{"line_number":1336,"context_line":"                return cleanup(*args, **kwargs)"},{"line_number":1337,"context_line":""},{"line_number":1338,"context_line":"        wrapper.reset \u003d functools.partial(reset, wrapper)"},{"line_number":1339,"context_line":"        return wrapper"},{"line_number":1340,"context_line":"    return outter_wrapper"}],"source_content_type":"text/x-python","patch_set":6,"id":"3f79a3b5_c457dc58","line":1338,"updated":"2018-10-26 16:27:48.000000000","message":"Bikeshed:\n\nwrapper.reset \u003d lambda: reset(wrapper)","commit_id":"a13ecf680e4414fd788b46afc528e552a468cafc"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"8aaae770497552835328b82f688ed7f7fb4ad53d","unresolved":false,"context_lines":[{"line_number":1335,"context_line":"            if cleanup:"},{"line_number":1336,"context_line":"                return cleanup(*args, **kwargs)"},{"line_number":1337,"context_line":""},{"line_number":1338,"context_line":"        wrapper.reset \u003d functools.partial(reset, wrapper)"},{"line_number":1339,"context_line":"        return wrapper"},{"line_number":1340,"context_line":"    return outter_wrapper"}],"source_content_type":"text/x-python","patch_set":6,"id":"3f79a3b5_921a8268","line":1338,"in_reply_to":"3f79a3b5_c457dc58","updated":"2018-10-26 17:33:56.000000000","message":"hehe i think that woudl be slower\nfunctools is already importated so using partial\ndoes not add any new dependecy and is likely\nmore idiomatic then lambdas here but either would work.","commit_id":"a13ecf680e4414fd788b46afc528e552a468cafc"},{"author":{"_account_id":7166,"name":"Sylvain Bauza","email":"sbauza@redhat.com","username":"sbauza"},"change_message_id":"3a354c8d9f4252ef7f72aeaa230d4984e292c602","unresolved":false,"context_lines":[{"line_number":1314,"context_line":"            yield [stack.enter_context(c) for c in contexts]"},{"line_number":1315,"context_line":""},{"line_number":1316,"context_line":""},{"line_number":1317,"context_line":"def run_once(message, logger, cleanup\u003dNone):"},{"line_number":1318,"context_line":"    \"\"\"This is a utility decorator to ensure a function is run"},{"line_number":1319,"context_line":"    once and only once in any interpreter instance. The decorated"},{"line_number":1320,"context_line":"    function object can be reset by calling its reset function."}],"source_content_type":"text/x-python","patch_set":10,"id":"3f79a3b5_b8cfc324","line":1317,"range":{"start_line":1317,"start_character":22,"end_line":1317,"end_character":28},"updated":"2018-11-05 13:25:19.000000000","message":"by default, we could provide a LOG.warning, but meh.","commit_id":"711b16b60a42beb4a9b9ea9d5f4ab02a4cadc77f"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"2226405e3b228495deb11fe507cc59700bda4cf5","unresolved":false,"context_lines":[{"line_number":1314,"context_line":"            yield [stack.enter_context(c) for c in contexts]"},{"line_number":1315,"context_line":""},{"line_number":1316,"context_line":""},{"line_number":1317,"context_line":"def run_once(message, logger, cleanup\u003dNone):"},{"line_number":1318,"context_line":"    \"\"\"This is a utility decorator to ensure a function is run"},{"line_number":1319,"context_line":"    once and only once in any interpreter instance. The decorated"},{"line_number":1320,"context_line":"    function object can be reset by calling its reset function."}],"source_content_type":"text/x-python","patch_set":10,"id":"3f79a3b5_703605eb","line":1317,"range":{"start_line":1317,"start_character":22,"end_line":1317,"end_character":28},"in_reply_to":"3f79a3b5_1b9b5179","updated":"2018-11-06 13:42:41.000000000","message":"Another alternative that is a bit more generic but more chatty as well.\n\n  def run_once(redirect_later_calls_to, cleanup\u003dNone)\n\npossible usage:\n\n  @run_once(redirect_later_calls_to\u003dlambda:LOG.warning(\u0027called more than once\u0027)\n  def ....\n\n\nWe can even pass the original parameters of the call to  redirect_later_calls_to() so it would be a real redirect and not just a replacement call.","commit_id":"711b16b60a42beb4a9b9ea9d5f4ab02a4cadc77f"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"cfb76c6ad55a3695d135a738d8b0062838f8fb2e","unresolved":false,"context_lines":[{"line_number":1314,"context_line":"            yield [stack.enter_context(c) for c in contexts]"},{"line_number":1315,"context_line":""},{"line_number":1316,"context_line":""},{"line_number":1317,"context_line":"def run_once(message, logger, cleanup\u003dNone):"},{"line_number":1318,"context_line":"    \"\"\"This is a utility decorator to ensure a function is run"},{"line_number":1319,"context_line":"    once and only once in any interpreter instance. The decorated"},{"line_number":1320,"context_line":"    function object can be reset by calling its reset function."}],"source_content_type":"text/x-python","patch_set":10,"id":"3f79a3b5_1b9b5179","line":1317,"range":{"start_line":1317,"start_character":22,"end_line":1317,"end_character":28},"in_reply_to":"3f79a3b5_b8cfc324","updated":"2018-11-05 14:22:00.000000000","message":"that would default all logs to use the logger form the utils module rather the the LOG logger form the module where the decorator was used which we may not want.\n\nbut yes that would be one option. for now i wanted to make it a requirement that you pass the logger. in general i would default to LOG.debug or LOG.info if we were to have a default as in general i dont think a warning would be required if the function was called twice but i do think a log is useful if troubleshooting an issue.","commit_id":"711b16b60a42beb4a9b9ea9d5f4ab02a4cadc77f"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"2226405e3b228495deb11fe507cc59700bda4cf5","unresolved":false,"context_lines":[{"line_number":1323,"context_line":"        @functools.wraps(func)"},{"line_number":1324,"context_line":"        def wrapper(*args, **kwargs):"},{"line_number":1325,"context_line":"            if not wrapper.called:"},{"line_number":1326,"context_line":"                ret \u003d func(*args, **kwargs)"},{"line_number":1327,"context_line":"                wrapper.called \u003d True"},{"line_number":1328,"context_line":"                return ret"},{"line_number":1329,"context_line":"            else:"},{"line_number":1330,"context_line":"                logger(message)"}],"source_content_type":"text/x-python","patch_set":10,"id":"3f79a3b5_d0791981","line":1327,"range":{"start_line":1326,"start_character":1,"end_line":1327,"end_character":37},"updated":"2018-11-06 13:42:41.000000000","message":"If func() throws an exception midflight then we don\u0027t set wrapper.called \u003d True so we would allow calling the function twice. I know that it might be matter of the definition of \u0027called\u0027 but for me called means that we did exec func(*arg, **kwargs) and it does not mean that it was also returned instead of raised.\n\nIf we need to differentiate between the two case then I suggest to call this usage of the flag \u0027success\u0027 instead of \u0027called\u0027 based on the common notation that exception is used mostly in fault cases. (Still feels shaky a bit)","commit_id":"711b16b60a42beb4a9b9ea9d5f4ab02a4cadc77f"},{"author":{"_account_id":7166,"name":"Sylvain Bauza","email":"sbauza@redhat.com","username":"sbauza"},"change_message_id":"3a354c8d9f4252ef7f72aeaa230d4984e292c602","unresolved":false,"context_lines":[{"line_number":1327,"context_line":"                wrapper.called \u003d True"},{"line_number":1328,"context_line":"                return ret"},{"line_number":1329,"context_line":"            else:"},{"line_number":1330,"context_line":"                logger(message)"},{"line_number":1331,"context_line":"        wrapper.called \u003d False"},{"line_number":1332,"context_line":""},{"line_number":1333,"context_line":"        def reset(wrapper, *args, **kwargs):"}],"source_content_type":"text/x-python","patch_set":10,"id":"3f79a3b5_588c6fd7","line":1330,"updated":"2018-11-05 13:25:19.000000000","message":"note, here it could be possible to create an exception to make sure the outer function would be stopped:\n\ndef raise_exc(message):\n  raise Exception(message)\n\n@run_once(\"boooo\", logger\u003draise_exc(\"booooh\"))\ndef blah()\n  ...","commit_id":"711b16b60a42beb4a9b9ea9d5f4ab02a4cadc77f"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"cfb76c6ad55a3695d135a738d8b0062838f8fb2e","unresolved":false,"context_lines":[{"line_number":1327,"context_line":"                wrapper.called \u003d True"},{"line_number":1328,"context_line":"                return ret"},{"line_number":1329,"context_line":"            else:"},{"line_number":1330,"context_line":"                logger(message)"},{"line_number":1331,"context_line":"        wrapper.called \u003d False"},{"line_number":1332,"context_line":""},{"line_number":1333,"context_line":"        def reset(wrapper, *args, **kwargs):"}],"source_content_type":"text/x-python","patch_set":10,"id":"3f79a3b5_e6d91c55","line":1330,"in_reply_to":"3f79a3b5_588c6fd7","updated":"2018-11-05 14:22:00.000000000","message":"am yes the logger is just a callable accepting one argument so you could do\n\ndef raise_exc(exception):\n  raise exception\n\n@run_once(\"boooo\", logger\u003draise_exc)\ndef blah()\n\nif you did this you are a bad person and that would make me sad however i think its resonable for the contrack to be don\u0027t pass things that are not loggers to the logger paramater.\n\n\ndoing \n@run_once(\"boooo\", logger\u003draise_exc(\"booooh\"))\nwould raise the exception on import of the module\nso the interpreter will slap you with an uncaught exception if you do this which you kind of deserve :)\n\nbut yes totally valid observation.","commit_id":"711b16b60a42beb4a9b9ea9d5f4ab02a4cadc77f"},{"author":{"_account_id":7166,"name":"Sylvain Bauza","email":"sbauza@redhat.com","username":"sbauza"},"change_message_id":"3a354c8d9f4252ef7f72aeaa230d4984e292c602","unresolved":false,"context_lines":[{"line_number":1330,"context_line":"                logger(message)"},{"line_number":1331,"context_line":"        wrapper.called \u003d False"},{"line_number":1332,"context_line":""},{"line_number":1333,"context_line":"        def reset(wrapper, *args, **kwargs):"},{"line_number":1334,"context_line":"            wrapper.called \u003d False"},{"line_number":1335,"context_line":"            if cleanup:"},{"line_number":1336,"context_line":"                return cleanup(*args, **kwargs)"}],"source_content_type":"text/x-python","patch_set":10,"id":"3f79a3b5_5b446948","line":1333,"updated":"2018-11-05 13:25:19.000000000","message":"it\u0027s fine if we only decorating a function or a method, but you could also decorate a class, right? In this case, I\u0027m not super happy because in case the decorated class would already have a reset() method, then the class instance running the decorator would be not calling it, but rather the decorator\u0027s reset() function.\n\nThat said, the docstring above tells about decorating functions, so I\u0027m okay. Maybe just a follow-up change for amending the docstring and saying to not using this decorator for a decorated class ?","commit_id":"711b16b60a42beb4a9b9ea9d5f4ab02a4cadc77f"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"cfb76c6ad55a3695d135a738d8b0062838f8fb2e","unresolved":false,"context_lines":[{"line_number":1330,"context_line":"                logger(message)"},{"line_number":1331,"context_line":"        wrapper.called \u003d False"},{"line_number":1332,"context_line":""},{"line_number":1333,"context_line":"        def reset(wrapper, *args, **kwargs):"},{"line_number":1334,"context_line":"            wrapper.called \u003d False"},{"line_number":1335,"context_line":"            if cleanup:"},{"line_number":1336,"context_line":"                return cleanup(*args, **kwargs)"}],"source_content_type":"text/x-python","patch_set":10,"id":"3f79a3b5_8677083d","line":1333,"in_reply_to":"3f79a3b5_5b446948","updated":"2018-11-05 14:22:00.000000000","message":"well what would run once on a class mean.\n\nthe only reasonable thing i can think of would be to apply\nthe run once behavior to __call__ but you can do that by hand when you overload __call__.\n\nin general you should never assume a decorator can be applied to both classes and functions/methods. most decorators only apply to one of the other and in this case this one does not support classes.\n\ni can add a follow up change to either support auto wrapping call or raise and exception on import when decorating a class if people really want. i kindof hopped people would have common sense and only apply it to functions but if we want to be more defensive i can do that as a follow up patch.","commit_id":"711b16b60a42beb4a9b9ea9d5f4ab02a4cadc77f"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"2226405e3b228495deb11fe507cc59700bda4cf5","unresolved":false,"context_lines":[{"line_number":1332,"context_line":""},{"line_number":1333,"context_line":"        def reset(wrapper, *args, **kwargs):"},{"line_number":1334,"context_line":"            wrapper.called \u003d False"},{"line_number":1335,"context_line":"            if cleanup:"},{"line_number":1336,"context_line":"                return cleanup(*args, **kwargs)"},{"line_number":1337,"context_line":""},{"line_number":1338,"context_line":"        wrapper.reset \u003d functools.partial(reset, wrapper)"},{"line_number":1339,"context_line":"        return wrapper"}],"source_content_type":"text/x-python","patch_set":10,"id":"3f79a3b5_30888d6a","line":1336,"range":{"start_line":1335,"start_character":0,"end_line":1336,"end_character":47},"updated":"2018-11-06 13:42:41.000000000","message":"I suggest to document that the cleanup callable is run at reset() regardless if the wrapper function run at all. So the provided cleanup should not depend on that the decorated function called exactly once. It can be called once or zero times.","commit_id":"711b16b60a42beb4a9b9ea9d5f4ab02a4cadc77f"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"6b11a64de08c3972a32a3cdb981b7be7d759dc31","unresolved":false,"context_lines":[{"line_number":1333,"context_line":"                # reset if they want to re-execute the wrapped function."},{"line_number":1334,"context_line":"                try:"},{"line_number":1335,"context_line":"                    return func(*args, **kwargs)"},{"line_number":1336,"context_line":"                except Exception:"},{"line_number":1337,"context_line":"                    exc_info \u003d sys.exc_info()"},{"line_number":1338,"context_line":"                    six.reraise(*exc_info)"},{"line_number":1339,"context_line":"                finally:"},{"line_number":1340,"context_line":"                    wrapper.called \u003d True"},{"line_number":1341,"context_line":"            else:"}],"source_content_type":"text/x-python","patch_set":11,"id":"3f79a3b5_b036afb2","line":1338,"range":{"start_line":1336,"start_character":0,"end_line":1338,"end_character":42},"updated":"2018-11-08 10:00:40.000000000","message":"Why do we need this? Can we simply let the exception propagate without catching it and reraising it?","commit_id":"65610f01e8fa81a4c57615dbc02bf09dc4be6e68"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"6b11a64de08c3972a32a3cdb981b7be7d759dc31","unresolved":false,"context_lines":[{"line_number":1336,"context_line":"                except Exception:"},{"line_number":1337,"context_line":"                    exc_info \u003d sys.exc_info()"},{"line_number":1338,"context_line":"                    six.reraise(*exc_info)"},{"line_number":1339,"context_line":"                finally:"},{"line_number":1340,"context_line":"                    wrapper.called \u003d True"},{"line_number":1341,"context_line":"            else:"},{"line_number":1342,"context_line":"                logger(message)"},{"line_number":1343,"context_line":""}],"source_content_type":"text/x-python","patch_set":11,"id":"3f79a3b5_10452316","line":1340,"range":{"start_line":1339,"start_character":0,"end_line":1340,"end_character":41},"updated":"2018-11-08 10:00:40.000000000","message":"Thanks","commit_id":"65610f01e8fa81a4c57615dbc02bf09dc4be6e68"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"6b11a64de08c3972a32a3cdb981b7be7d759dc31","unresolved":false,"context_lines":[{"line_number":1353,"context_line":"            try:"},{"line_number":1354,"context_line":"                if cleanup and wrapper.called:"},{"line_number":1355,"context_line":"                    return cleanup(*args, **kwargs)"},{"line_number":1356,"context_line":"            except Exception:"},{"line_number":1357,"context_line":"                exc_info \u003d sys.exc_info()"},{"line_number":1358,"context_line":"                six.reraise(*exc_info)"},{"line_number":1359,"context_line":"            finally:"},{"line_number":1360,"context_line":"                wrapper.called \u003d False"},{"line_number":1361,"context_line":""}],"source_content_type":"text/x-python","patch_set":11,"id":"3f79a3b5_705577c0","line":1358,"range":{"start_line":1356,"start_character":1,"end_line":1358,"end_character":38},"updated":"2018-11-08 10:00:40.000000000","message":"ditto, I think we can delete this and let the exception propagate automatically","commit_id":"65610f01e8fa81a4c57615dbc02bf09dc4be6e68"}]}