)]}'
{"nova/db/sqlalchemy/api.py":[{"author":{"_account_id":11303,"name":"Markus Zoeller (markus_z)","email":"mzoeller@linux.vnet.ibm.com","username":"mzoeller"},"change_message_id":"7ac09cb912d4b150911cb632c8580dced5c11f02","unresolved":false,"context_lines":[{"line_number":818,"context_line":""},{"line_number":819,"context_line":"    # Delete the quotas, if needed."},{"line_number":820,"context_line":"    # Quota update happens in a separate transaction, so previous must have"},{"line_number":821,"context_line":"    # been committed first."},{"line_number":822,"context_line":"    for project_id, count in project_id_to_quota_count.iteritems():"},{"line_number":823,"context_line":"        try:"},{"line_number":824,"context_line":"            reservations \u003d quota.QUOTAS.reserve(context,"}],"source_content_type":"text/x-python","patch_set":1,"id":"da86d52c_3885e2ce","line":821,"updated":"2015-02-12 14:57:49.000000000","message":"First a short disclaimer, my experience on the DB layer is next to nothing and I\u0027m still an OpenStack newbie. That\u0027s why I don\u0027t -1 this although I have a tiny doubt:\n\nAs far as I can judge, the deletion of the floating IPs should be rolled back if the quotas couldn\u0027t be deleted, is this assumption right? Would this rollback happen when the deletion of the quotas is outside of the first transaction where the deletion of the floating IPs happens? Right now I would read the code like \"delete all or none of the floating IPs, after that delete as much related quotas as possible.\"\n\nMaybe you could find the time helping me to clarify this. I would then be able to do more and better reviews :)","commit_id":"01dfd19d1a810de6f913178e758ce3c6f3a8795f"},{"author":{"_account_id":11303,"name":"Markus Zoeller (markus_z)","email":"mzoeller@linux.vnet.ibm.com","username":"mzoeller"},"change_message_id":"4beb91aad8549ed8122f08e9bdb7ef8e2c5194b6","unresolved":false,"context_lines":[{"line_number":818,"context_line":""},{"line_number":819,"context_line":"    # Delete the quotas, if needed."},{"line_number":820,"context_line":"    # Quota update happens in a separate transaction, so previous must have"},{"line_number":821,"context_line":"    # been committed first."},{"line_number":822,"context_line":"    for project_id, count in project_id_to_quota_count.iteritems():"},{"line_number":823,"context_line":"        try:"},{"line_number":824,"context_line":"            reservations \u003d quota.QUOTAS.reserve(context,"}],"source_content_type":"text/x-python","patch_set":1,"id":"da86d52c_27593b35","line":821,"in_reply_to":"da86d52c_0eb3740a","updated":"2015-02-12 16:00:21.000000000","message":"Thank you, I understand the intention of this method and the background a bit better now. Hopefully I will find some time to read review 138215.\n\nBefore your change, the method looked like that an exception during the deletion of the quotas *should* lead to a rollback of the deletion of the floating IPs (but didn\u0027t because the model_query for soft_delete didn\u0027t use the session of line 801). What I try to say is that this patch set changes the (intended) rollback behavior of the floating IP deletion in case of a quota exception. This pseudo code is what I try to say:\n\n    session \u003d get_session()\n    with session.begin():\n        floating_ips \u003d db.query_ips(session\u003dsession)\n        db.delete(floating_ips, session\u003dsession)\n        quota.update() # an exc causes ip deletion rollback\n        quota.commit() # an exc causes ip deletion rollback\n\nThese are only some thoughts of a very short analysis of the code. Feel free to say \u0027nay\u0027 :) Thanks for your time.","commit_id":"01dfd19d1a810de6f913178e758ce3c6f3a8795f"},{"author":{"_account_id":11303,"name":"Markus Zoeller (markus_z)","email":"mzoeller@linux.vnet.ibm.com","username":"mzoeller"},"change_message_id":"1831f94719d86e6a15ddfac28e1130a120be5fd8","unresolved":false,"context_lines":[{"line_number":818,"context_line":""},{"line_number":819,"context_line":"    # Delete the quotas, if needed."},{"line_number":820,"context_line":"    # Quota update happens in a separate transaction, so previous must have"},{"line_number":821,"context_line":"    # been committed first."},{"line_number":822,"context_line":"    for project_id, count in project_id_to_quota_count.iteritems():"},{"line_number":823,"context_line":"        try:"},{"line_number":824,"context_line":"            reservations \u003d quota.QUOTAS.reserve(context,"}],"source_content_type":"text/x-python","patch_set":1,"id":"da86d52c_de55611f","line":821,"in_reply_to":"da86d52c_1e0379bf","updated":"2015-02-13 11:45:32.000000000","message":"Right, you mentioned that explicitly too. I have to listen more carefully. I think all doubts I had are swept away now. Many thanks for your time!","commit_id":"01dfd19d1a810de6f913178e758ce3c6f3a8795f"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"0d55f45e9a30e82282fdea35e8377dcbf13d717b","unresolved":false,"context_lines":[{"line_number":818,"context_line":""},{"line_number":819,"context_line":"    # Delete the quotas, if needed."},{"line_number":820,"context_line":"    # Quota update happens in a separate transaction, so previous must have"},{"line_number":821,"context_line":"    # been committed first."},{"line_number":822,"context_line":"    for project_id, count in project_id_to_quota_count.iteritems():"},{"line_number":823,"context_line":"        try:"},{"line_number":824,"context_line":"            reservations \u003d quota.QUOTAS.reserve(context,"}],"source_content_type":"text/x-python","patch_set":1,"id":"da86d52c_f25c1e1c","line":821,"in_reply_to":"da86d52c_27593b35","updated":"2015-02-13 09:48:06.000000000","message":"I\u0027ve never thought of using nested transactions this way. That\u0027s a really interesting idea which might fly in certain circumstances.\n\nHowever, that isn\u0027t what it was doing before. Notice that it created the session, then it didn\u0027t use it to query floating ips (model_query implicitly created a new session), and it didn\u0027t use it for the delete (model_query implicitly created yet another new session). So both of these operations happened in their own, entirely separate transactions, and were therefore committed before the quota update. The top level session was created, but wasn\u0027t actually used at all :/","commit_id":"01dfd19d1a810de6f913178e758ce3c6f3a8795f"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"f138043d67f68ff01c4a69147e519d921fb26b66","unresolved":false,"context_lines":[{"line_number":818,"context_line":""},{"line_number":819,"context_line":"    # Delete the quotas, if needed."},{"line_number":820,"context_line":"    # Quota update happens in a separate transaction, so previous must have"},{"line_number":821,"context_line":"    # been committed first."},{"line_number":822,"context_line":"    for project_id, count in project_id_to_quota_count.iteritems():"},{"line_number":823,"context_line":"        try:"},{"line_number":824,"context_line":"            reservations \u003d quota.QUOTAS.reserve(context,"}],"source_content_type":"text/x-python","patch_set":1,"id":"da86d52c_0eb3740a","line":821,"in_reply_to":"da86d52c_3885e2ce","updated":"2015-02-12 15:28:13.000000000","message":"No, you\u0027re quite right: these things should be done in a single transaction. However, this patch doesn\u0027t change that, and I did note it explicitly in the commit message. This patch fixes some, but not all of the problems in this function.\n\nFixing the quota issue isn\u0027t straightforward, yet. Note that the quota api crosses out of this module, and we currently have no mechanism which allows us to propagate the current transaction. This change might get us there, though:\n\nhttps://review.openstack.org/#/c/138215/","commit_id":"01dfd19d1a810de6f913178e758ce3c6f3a8795f"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"bcef6c102fd745a17192fb1cc11c4fa3331b19b0","unresolved":false,"context_lines":[{"line_number":818,"context_line":""},{"line_number":819,"context_line":"    # Delete the quotas, if needed."},{"line_number":820,"context_line":"    # Quota update happens in a separate transaction, so previous must have"},{"line_number":821,"context_line":"    # been committed first."},{"line_number":822,"context_line":"    for project_id, count in project_id_to_quota_count.iteritems():"},{"line_number":823,"context_line":"        try:"},{"line_number":824,"context_line":"            reservations \u003d quota.QUOTAS.reserve(context,"}],"source_content_type":"text/x-python","patch_set":1,"id":"da86d52c_1e0379bf","line":821,"in_reply_to":"da86d52c_58f18900","updated":"2015-02-13 11:35:55.000000000","message":"It\u0027s quite a while since I wrote this, but IIRC the quota call needs to \u0027see\u0027 the deletion of the floating ips, or it doesn\u0027t work in some manner I don\u0027t recall. Ideally the quota call would use the same transaction, but given that it can\u0027t (and assuming my recollection is correct) moving it \u0027inside\u0027 the enclosing transaction would not work. This is because the enclosing transaction wouldn\u0027t have been committed yet, and therefore wouldn\u0027t be visible to the quota call.","commit_id":"01dfd19d1a810de6f913178e758ce3c6f3a8795f"},{"author":{"_account_id":11303,"name":"Markus Zoeller (markus_z)","email":"mzoeller@linux.vnet.ibm.com","username":"mzoeller"},"change_message_id":"b939b265589ae8e66e7e45ed1d0b79192be314aa","unresolved":false,"context_lines":[{"line_number":818,"context_line":""},{"line_number":819,"context_line":"    # Delete the quotas, if needed."},{"line_number":820,"context_line":"    # Quota update happens in a separate transaction, so previous must have"},{"line_number":821,"context_line":"    # been committed first."},{"line_number":822,"context_line":"    for project_id, count in project_id_to_quota_count.iteritems():"},{"line_number":823,"context_line":"        try:"},{"line_number":824,"context_line":"            reservations \u003d quota.QUOTAS.reserve(context,"}],"source_content_type":"text/x-python","patch_set":1,"id":"da86d52c_58f18900","line":821,"in_reply_to":"da86d52c_f25c1e1c","updated":"2015-02-13 10:38:39.000000000","message":"\u003e The top level session was created, but wasn\u0027t actually used at all\n\nYes, I\u0027m with you in this point and I think that your change should fix that (as it does right now). \n\nAs far as I understand, the commit of this transaction will happen *after* the \"with session.begin():\" block. That\u0027s why I think that moving the quota db interactions *after* this block isn\u0027t the right thing. When they are *within* the transaction block and throw an exception the transaction (of deleting floating ips) will rollback. \n\nI will try to write a unit test on Monday. Maybe this will express the behavior I expect from the code in a better way.","commit_id":"01dfd19d1a810de6f913178e758ce3c6f3a8795f"}]}