)]}'
{"quantum/plugins/nicira/QuantumPlugin.py":[{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"change_message_id":"ae4a068cf52347f47c5cae5a59d2156dff0b50f4","unresolved":false,"context_lines":[{"line_number":1439,"context_line":"            LOG.debug(_(\"Update port request: %s\"), port)"},{"line_number":1440,"context_line":"            nvp_port_id \u003d self._nvp_get_port_id("},{"line_number":1441,"context_line":"                context, self.default_cluster, ret_port)"},{"line_number":1442,"context_line":"            if nvp_port_id:"},{"line_number":1443,"context_line":"                nvplib.update_port(self.default_cluster,"},{"line_number":1444,"context_line":"                                   ret_port[\u0027network_id\u0027],"},{"line_number":1445,"context_line":"                                   nvp_port_id, id, tenant_id,"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAOn%2F%2FxVQ%3D","line":1442,"updated":"2013-04-23 21:50:39.000000000","message":"have you thought of raising the exception (rather than masking it in _nvp_get_port_id) and wrapping the update+get_status operations in a try-except? It would make the code slightly more readable.\n\nCatching Exception is not ideal, but I guess that\u0027s a different story.","commit_id":"185336f83af00715c95cc9e046a44b406f965b33"},{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"unresolved":false,"context_lines":[{"line_number":1439,"context_line":"            LOG.debug(_(\"Update port request: %s\"), port)"},{"line_number":1440,"context_line":"            nvp_port_id \u003d self._nvp_get_port_id("},{"line_number":1441,"context_line":"                context, self.default_cluster, ret_port)"},{"line_number":1442,"context_line":"            if nvp_port_id:"},{"line_number":1443,"context_line":"                nvplib.update_port(self.default_cluster,"},{"line_number":1444,"context_line":"                                   ret_port[\u0027network_id\u0027],"},{"line_number":1445,"context_line":"                                   nvp_port_id, id, tenant_id,"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAOn%2F%2Fwvw%3D","line":1442,"in_reply_to":"AAAAOn%2F%2FxNM%3D","updated":"2013-04-24 11:47:36.000000000","message":"Yes, in this particular case, if _nvp_get_port_id raised an exception and we got rid of the if/else statement, the port status would be set in the except block.\n\nRegarding the Exception, if multiple exceptions may occur and they need to be handled equally, then it makes sense to catch the base one. You mentioned a timeout, would it make sense to retry?","commit_id":"185336f83af00715c95cc9e046a44b406f965b33"},{"author":{"_account_id":748,"name":"Armando Migliaccio","email":"armamig@gmail.com","username":"armando-migliaccio"},"unresolved":false,"context_lines":[{"line_number":1439,"context_line":"            LOG.debug(_(\"Update port request: %s\"), port)"},{"line_number":1440,"context_line":"            nvp_port_id \u003d self._nvp_get_port_id("},{"line_number":1441,"context_line":"                context, self.default_cluster, ret_port)"},{"line_number":1442,"context_line":"            if nvp_port_id:"},{"line_number":1443,"context_line":"                nvplib.update_port(self.default_cluster,"},{"line_number":1444,"context_line":"                                   ret_port[\u0027network_id\u0027],"},{"line_number":1445,"context_line":"                                   nvp_port_id, id, tenant_id,"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAOn%2F%2Fwwg%3D","line":1442,"in_reply_to":"AAAAOn%2F%2FxOM%3D","updated":"2013-04-24 11:47:36.000000000","message":"Changing the behavior of a method has consequences, but that far out to the dhcp agent? It sounds like there\u0027s some serious leak of implementation details. Maybe my comment was unclear...I was suggesting to let the exception be raised in NvpPluginV2._nvp_get_port_id, and have a more linear flow in the rest of the plugin. I noticed that the plugin code has several if/else statements that check for None port_id; that\u0027s okay if a missing value is acceptable, otherwise, if the value is to be expected, throwing an exception may be more appropriate. However from what I see in the code it looks like there are times where a None port is ok, and other times where an exception is re-raised to upper layer. So I guess it all boils down to style preferences, even though consistency is paramount.","commit_id":"185336f83af00715c95cc9e046a44b406f965b33"},{"author":{"_account_id":261,"name":"Salvatore Orlando","email":"salv.orlando@gmail.com","username":"salvatore-orlando"},"unresolved":false,"context_lines":[{"line_number":1439,"context_line":"            LOG.debug(_(\"Update port request: %s\"), port)"},{"line_number":1440,"context_line":"            nvp_port_id \u003d self._nvp_get_port_id("},{"line_number":1441,"context_line":"                context, self.default_cluster, ret_port)"},{"line_number":1442,"context_line":"            if nvp_port_id:"},{"line_number":1443,"context_line":"                nvplib.update_port(self.default_cluster,"},{"line_number":1444,"context_line":"                                   ret_port[\u0027network_id\u0027],"},{"line_number":1445,"context_line":"                                   nvp_port_id, id, tenant_id,"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAOn%2F%2FxNM%3D","line":1442,"in_reply_to":"AAAAOn%2F%2FxOM%3D","updated":"2013-04-24 00:22:48.000000000","message":"I think Armando was just suggesting to set the port in ERROR in the except block.\n\nI would not change the Exception at line 1461 instead.\nI know the wide catch block is not ideal, but get_port_status can fail also because of a timeout, and we don\u0027t want the whole update operation to fail because of this. the timeout will result in a NvpApiClientException while we (incorrectly) cancel the ResourceNotFoundException (which extends the above) and re-raise a NotFound which is a totally unrelated QuantumException.\n\nSo we either catch multiple exception type or we just keep it broad.","commit_id":"185336f83af00715c95cc9e046a44b406f965b33"},{"author":{"_account_id":4395,"name":"Aaron Rosen","email":"aaronorosen@gmail.com","username":"arosen"},"change_message_id":"c43462b7a42cee24872fead3598fff609e0f333f","unresolved":false,"context_lines":[{"line_number":1439,"context_line":"            LOG.debug(_(\"Update port request: %s\"), port)"},{"line_number":1440,"context_line":"            nvp_port_id \u003d self._nvp_get_port_id("},{"line_number":1441,"context_line":"                context, self.default_cluster, ret_port)"},{"line_number":1442,"context_line":"            if nvp_port_id:"},{"line_number":1443,"context_line":"                nvplib.update_port(self.default_cluster,"},{"line_number":1444,"context_line":"                                   ret_port[\u0027network_id\u0027],"},{"line_number":1445,"context_line":"                                   nvp_port_id, id, tenant_id,"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAOn%2F%2FxOM%3D","line":1442,"in_reply_to":"AAAAOn%2F%2FxVQ%3D","updated":"2013-04-23 23:38:08.000000000","message":"Unfortunately if an exception is raised here it will cause the dhcp agent not to work anymore that\u0027s why we\u0027re not raising currently.  IMO setting the port status to be ERROR should be good enough, the only thing is that one cannot update is the actual admin_state_up  in NVP but imo that\u0027s okay too because the port doesn\u0027t exist in NVP anymore.\n\nI agree we should change the except on line 1461 to just catch NotFound instead of Exception though. I rather leave that for another day though if you are okay with that.","commit_id":"185336f83af00715c95cc9e046a44b406f965b33"},{"author":{"_account_id":642,"name":"Matt Dietz","email":"matthew.dietz@gmail.com","username":"cerberus"},"unresolved":false,"context_lines":[{"line_number":1459,"context_line":"                        self.default_cluster, ret_port[\u0027network_id\u0027],"},{"line_number":1460,"context_line":"                        nvp_port_id)"},{"line_number":1461,"context_line":"                except Exception:"},{"line_number":1462,"context_line":"                    LOG.warn(_(\"Unable to retrieve port status for:%s.\"),"},{"line_number":1463,"context_line":"                             nvp_port_id)"},{"line_number":1464,"context_line":""},{"line_number":1465,"context_line":"            # If nvp_port_id is not in database or in nvp put in error state."}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAOn%2F%2FwcQ%3D","line":1462,"updated":"2013-04-24 16:37:27.000000000","message":"Because I like being a dissenter, I\u0027d actually prefer we explicitly catch the exceptions we know will occur rather than a blanket case like this. It\u0027s been stated a million times in reviews before, but catching Exception can hide real, underlying problems. \n\nI imagine there\u0027s a risk of showing the client a 500 here, right? I\u0027d rather run that risk than have a constantly recurring problem (NVP died in some unrecoverable way) and the content of the exception is tossed out the window. Exception handling like this usually ends up in me having to hot patch the code and bounce Quantum to find out what\u0027s really going on ;-)","commit_id":"185336f83af00715c95cc9e046a44b406f965b33"},{"author":{"_account_id":4395,"name":"Aaron Rosen","email":"aaronorosen@gmail.com","username":"arosen"},"change_message_id":"c43462b7a42cee24872fead3598fff609e0f333f","unresolved":false,"context_lines":[{"line_number":1459,"context_line":"                        self.default_cluster, ret_port[\u0027network_id\u0027],"},{"line_number":1460,"context_line":"                        nvp_port_id)"},{"line_number":1461,"context_line":"                except Exception:"},{"line_number":1462,"context_line":"                    LOG.warn(_(\"Unable to retrieve port status for:%s.\"),"},{"line_number":1463,"context_line":"                             nvp_port_id)"},{"line_number":1464,"context_line":""},{"line_number":1465,"context_line":"            # If nvp_port_id is not in database or in nvp put in error state."}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAOn%2F%2Fo9g%3D","line":1462,"in_reply_to":"AAAAOn%2F%2FtpI%3D","updated":"2013-04-30 22:49:08.000000000","message":"I agree. For now updating to LOG.exception().","commit_id":"185336f83af00715c95cc9e046a44b406f965b33"},{"author":{"_account_id":642,"name":"Matt Dietz","email":"matthew.dietz@gmail.com","username":"cerberus"},"unresolved":false,"context_lines":[{"line_number":1459,"context_line":"                        self.default_cluster, ret_port[\u0027network_id\u0027],"},{"line_number":1460,"context_line":"                        nvp_port_id)"},{"line_number":1461,"context_line":"                except Exception:"},{"line_number":1462,"context_line":"                    LOG.warn(_(\"Unable to retrieve port status for:%s.\"),"},{"line_number":1463,"context_line":"                             nvp_port_id)"},{"line_number":1464,"context_line":""},{"line_number":1465,"context_line":"            # If nvp_port_id is not in database or in nvp put in error state."}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAOn%2F%2FtpI%3D","line":1462,"in_reply_to":"AAAAOn%2F%2FuSg%3D","updated":"2013-04-26 15:43:01.000000000","message":"Should probably use LOG.exception in any case, then\n\nI\u0027m still not a fan of broad exception handling, but yes, bare minimum is to log the exception as well, IMO","commit_id":"185336f83af00715c95cc9e046a44b406f965b33"},{"author":{"_account_id":2592,"name":"Mark McClain","email":"mark@mcclain.xyz","username":"markmcclain"},"unresolved":false,"context_lines":[{"line_number":1459,"context_line":"                        self.default_cluster, ret_port[\u0027network_id\u0027],"},{"line_number":1460,"context_line":"                        nvp_port_id)"},{"line_number":1461,"context_line":"                except Exception:"},{"line_number":1462,"context_line":"                    LOG.warn(_(\"Unable to retrieve port status for:%s.\"),"},{"line_number":1463,"context_line":"                             nvp_port_id)"},{"line_number":1464,"context_line":""},{"line_number":1465,"context_line":"            # If nvp_port_id is not in database or in nvp put in error state."}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAOn%2F%2FuSg%3D","line":1462,"in_reply_to":"AAAAOn%2F%2FwcQ%3D","updated":"2013-04-26 03:40:50.000000000","message":"A generic catch is ok here as long as the exception reason is added to the warning message.","commit_id":"185336f83af00715c95cc9e046a44b406f965b33"},{"author":{"_account_id":1653,"name":"garyk","email":"gkotton@vmware.com","username":"garyk"},"unresolved":false,"context_lines":[{"line_number":1272,"context_line":"                    # able to retrieve the status."},{"line_number":1273,"context_line":"                    ret_port[\u0027status\u0027] \u003d nvplib.get_port_status("},{"line_number":1274,"context_line":"                        self.cluster, ret_port[\u0027network_id\u0027],"},{"line_number":1275,"context_line":"                        nvp_port_id)"},{"line_number":1276,"context_line":"                # FIXME(arosen) improve exception handling."},{"line_number":1277,"context_line":"                except Exception:"},{"line_number":1278,"context_line":"                    ret_port[\u0027status\u0027] \u003d constants.PORT_STATUS_ERROR"}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAAOn%2F%2FoVA%3D","line":1275,"updated":"2013-05-01 06:35:31.000000000","message":"would it be worth considering having the update_port return the status?","commit_id":"5b53b0960a7bfccf76857a76bd8cb7432f60786e"},{"author":{"_account_id":4395,"name":"Aaron Rosen","email":"aaronorosen@gmail.com","username":"arosen"},"change_message_id":"c43462b7a42cee24872fead3598fff609e0f333f","unresolved":false,"context_lines":[{"line_number":1272,"context_line":"                    # able to retrieve the status."},{"line_number":1273,"context_line":"                    ret_port[\u0027status\u0027] \u003d nvplib.get_port_status("},{"line_number":1274,"context_line":"                        self.cluster, ret_port[\u0027network_id\u0027],"},{"line_number":1275,"context_line":"                        nvp_port_id)"},{"line_number":1276,"context_line":"                # FIXME(arosen) improve exception handling."},{"line_number":1277,"context_line":"                except Exception:"},{"line_number":1278,"context_line":"                    ret_port[\u0027status\u0027] \u003d constants.PORT_STATUS_ERROR"}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAAOn%2F%2Fht4%3D","line":1275,"in_reply_to":"AAAAOn%2F%2FoVA%3D","updated":"2013-05-06 19:58:54.000000000","message":"Perhaps but that should be in a different patch if we decide to do that.","commit_id":"5b53b0960a7bfccf76857a76bd8cb7432f60786e"},{"author":{"_account_id":1653,"name":"garyk","email":"gkotton@vmware.com","username":"garyk"},"unresolved":false,"context_lines":[{"line_number":1276,"context_line":"                # FIXME(arosen) improve exception handling."},{"line_number":1277,"context_line":"                except Exception:"},{"line_number":1278,"context_line":"                    ret_port[\u0027status\u0027] \u003d constants.PORT_STATUS_ERROR"},{"line_number":1279,"context_line":"                    LOG.exception(_(\"Unable to update port id:%s.\"),"},{"line_number":1280,"context_line":"                                  nvp_port_id)"},{"line_number":1281,"context_line":""},{"line_number":1282,"context_line":"            # If nvp_port_id is not in database or in nvp put in error state."}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAAOn%2F%2FoVQ%3D","line":1279,"updated":"2013-05-01 06:35:31.000000000","message":": %s","commit_id":"5b53b0960a7bfccf76857a76bd8cb7432f60786e"},{"author":{"_account_id":4395,"name":"Aaron Rosen","email":"aaronorosen@gmail.com","username":"arosen"},"change_message_id":"c43462b7a42cee24872fead3598fff609e0f333f","unresolved":false,"context_lines":[{"line_number":1276,"context_line":"                # FIXME(arosen) improve exception handling."},{"line_number":1277,"context_line":"                except Exception:"},{"line_number":1278,"context_line":"                    ret_port[\u0027status\u0027] \u003d constants.PORT_STATUS_ERROR"},{"line_number":1279,"context_line":"                    LOG.exception(_(\"Unable to update port id:%s.\"),"},{"line_number":1280,"context_line":"                                  nvp_port_id)"},{"line_number":1281,"context_line":""},{"line_number":1282,"context_line":"            # If nvp_port_id is not in database or in nvp put in error state."}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAAOn%2F%2FhuI%3D","line":1279,"in_reply_to":"AAAAOn%2F%2FoVQ%3D","updated":"2013-05-06 19:58:54.000000000","message":"Done","commit_id":"5b53b0960a7bfccf76857a76bd8cb7432f60786e"}]}