Douglas Schilling Landgraf has uploaded a new change for review.
Change subject: [WIP] Adding flowID ......................................................................
[WIP] Adding flowID
This patch will implement the flowID schema. The idea is provide a mechanism to identify the transactions between ovirt engine and vdsm.
Change-Id: I6c818d7a90fea41663fd48620ac9718451a8662f --- M vdsm/clientIF.py 1 file changed, 30 insertions(+), 0 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/21/1221/1 -- To view, visit http://gerrit.ovirt.org/1221 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange Gerrit-Change-Id: I6c818d7a90fea41663fd48620ac9718451a8662f Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Douglas Schilling Landgraf dougsland@redhat.com
Dan Kenigsberg has posted comments on this change.
Change subject: [WIP] Adding flowID ......................................................................
Patch Set 1: I would prefer that you didn't submit this
(1 inline comment)
.................................................... File vdsm/clientIF.py Line 648: try: I have imagined something more generic, embedded into wrapApiMethod and applying to all verbs.
In any case, you cannot change self.log this way - it is raceful and would quickly explode in length (note that you add an Adapter on every api call, never removing).
Instead, I think that you should copy the flowID to threadLocal, and have one constant LogAdapter that reads flowID from there when needed.
-- To view, visit http://gerrit.ovirt.org/1221 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I6c818d7a90fea41663fd48620ac9718451a8662f Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Douglas Schilling Landgraf dougsland@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com
Dan Kenigsberg has posted comments on this change.
Change subject: [WIP] Adding flowID ......................................................................
Patch Set 2: I would prefer that you didn't submit this
(2 inline comments)
.................................................... File vdsm/clientIF.py Line 75: argsStr = str(args).split(",") I do not understand what's going on here. Are you converting the arguments to a string and then back to a dictionary? Why? you should look for a dict in args list.
Line 100: logStr += " flowID [%s]" % argsDict['flowID'] logging flowID on API entry point is nice, but was deemed not enough. Please consider writing a LogAdapter that prints it from threadLocal.
-- To view, visit http://gerrit.ovirt.org/1221 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I6c818d7a90fea41663fd48620ac9718451a8662f Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Douglas Schilling Landgraf dougsland@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Douglas Schilling Landgraf dougsland@redhat.com
Douglas Schilling Landgraf has posted comments on this change.
Change subject: [WIP] Adding flowID ......................................................................
Patch Set 2: (2 inline comments)
.................................................... File vdsm/clientIF.py Line 75: argsStr = str(args).split(",") humm, I remember it was a tuple instead of dict. /me going to check again. I just converted to str, then dict because it could safe time in the future if we need to change this code again. (Avoid to read each element of tuple every time we need it to locate the argument and then read +1 position to get the value, with dict it`s easy to manage such situation).
Line 100: logStr += " flowID [%s]" % argsDict['flowID'] I thought it was enough. Going to prepare a new version. thanks
-- To view, visit http://gerrit.ovirt.org/1221 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I6c818d7a90fea41663fd48620ac9718451a8662f Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Douglas Schilling Landgraf dougsland@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Douglas Schilling Landgraf dougsland@redhat.com
Dan Kenigsberg has posted comments on this change.
Change subject: [WIP] Adding flowID ......................................................................
Patch Set 2: (2 inline comments)
.................................................... File vdsm/clientIF.py Line 75: argsStr = str(args).split(",") parsing strings is slow and error prone. We have easily accessible in-memory objects, let us use them.
Line 100: logStr += " flowID [%s]" % argsDict['flowID'] As I told you privately, you can start with logging on API entry.
-- To view, visit http://gerrit.ovirt.org/1221 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I6c818d7a90fea41663fd48620ac9718451a8662f Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Douglas Schilling Landgraf dougsland@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Douglas Schilling Landgraf dougsland@redhat.com
Dan Kenigsberg has posted comments on this change.
Change subject: Adding flowID ......................................................................
Patch Set 4: Looks good to me, but someone else must approve
-- To view, visit http://gerrit.ovirt.org/1221 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I6c818d7a90fea41663fd48620ac9718451a8662f Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Douglas Schilling Landgraf dougsland@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Douglas Schilling Landgraf dougsland@redhat.com Gerrit-Reviewer: Moti Asayag masayag@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com
Moti Asayag has posted comments on this change.
Change subject: Adding flowID ......................................................................
Patch Set 4: (1 inline comment)
.................................................... Commit Message Line 10: mechanism to identify the transactions between ovirt engine and vdsm. Just to sharpen the description, the same flowID could be shared between several API calls between ovirt engine and vdsm in order to identify the entire scope of the backend action (which possibly consisted of several VSDM calls).
-- To view, visit http://gerrit.ovirt.org/1221 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I6c818d7a90fea41663fd48620ac9718451a8662f Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Douglas Schilling Landgraf dougsland@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Douglas Schilling Landgraf dougsland@redhat.com Gerrit-Reviewer: Moti Asayag masayag@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com
Saggi Mizrahi has posted comments on this change.
Change subject: Adding flowID ......................................................................
Patch Set 4: I would prefer that you didn't submit this
(1 inline comment)
I would have put -2 if the patch wasn't so tiny.
Anyway, I still think flowID is useless but as long as it doesn't get in my way and gets deprecated when we move to REST/QMF I don't care if there is another stupid UUID in the logs.
.................................................... File vdsm/BindingXMLRPC.py Line 139: threadLocal.flowID = self.headers.get('X-FlowID') * Thread local storage is evil * X- prefix is deprecated (http://tools.ietf.org/html/draft-ietf-appsawg-xdash-02) * Header name should be in a constant
-- To view, visit http://gerrit.ovirt.org/1221 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I6c818d7a90fea41663fd48620ac9718451a8662f Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Douglas Schilling Landgraf dougsland@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Douglas Schilling Landgraf dougsland@redhat.com Gerrit-Reviewer: Moti Asayag masayag@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com
Douglas Schilling Landgraf has posted comments on this change.
Change subject: Adding flowID ......................................................................
Patch Set 5: Verified
-- To view, visit http://gerrit.ovirt.org/1221 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I6c818d7a90fea41663fd48620ac9718451a8662f Gerrit-PatchSet: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Douglas Schilling Landgraf dougsland@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Douglas Schilling Landgraf dougsland@redhat.com Gerrit-Reviewer: Moti Asayag masayag@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com
Douglas Schilling Landgraf has posted comments on this change.
Change subject: Adding flowID ......................................................................
Patch Set 4: (2 inline comments)
.................................................... Commit Message Line 10: mechanism to identify the transactions between ovirt engine and vdsm. Done
.................................................... File vdsm/BindingXMLRPC.py Line 139: threadLocal.flowID = self.headers.get('X-FlowID') Done
-- To view, visit http://gerrit.ovirt.org/1221 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I6c818d7a90fea41663fd48620ac9718451a8662f Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Douglas Schilling Landgraf dougsland@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Douglas Schilling Landgraf dougsland@redhat.com Gerrit-Reviewer: Moti Asayag masayag@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com
Dan Kenigsberg has posted comments on this change.
Change subject: Adding flowID ......................................................................
Patch Set 5: I would prefer that you didn't submit this
(1 inline comment)
please remember my little refactoring patch. you may want to take it *before* your own patch, so that you do not have to duplicate code.
.................................................... File vdsm/BindingXMLRPC.py Line 139: threadLocal.flowID = self.headers.get(constants.HTTP_HEADER_FLOWID) it makes sense for this to be a CONSTANT, particularly if used twice. However, there is no justification to throw it into the global trash can called "constants". Use a module-local _CONSTANT instead.
-- To view, visit http://gerrit.ovirt.org/1221 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I6c818d7a90fea41663fd48620ac9718451a8662f Gerrit-PatchSet: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Douglas Schilling Landgraf dougsland@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Douglas Schilling Landgraf dougsland@redhat.com Gerrit-Reviewer: Moti Asayag masayag@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com
Douglas Schilling Landgraf has posted comments on this change.
Change subject: Adding flowID ......................................................................
Patch Set 6: Verified
-- To view, visit http://gerrit.ovirt.org/1221 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I6c818d7a90fea41663fd48620ac9718451a8662f Gerrit-PatchSet: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Douglas Schilling Landgraf dougsland@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Douglas Schilling Landgraf dougsland@redhat.com Gerrit-Reviewer: Moti Asayag masayag@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com
Moti Asayag has posted comments on this change.
Change subject: Adding flowID ......................................................................
Patch Set 6: Looks good to me, but someone else must approve
-- To view, visit http://gerrit.ovirt.org/1221 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I6c818d7a90fea41663fd48620ac9718451a8662f Gerrit-PatchSet: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Douglas Schilling Landgraf dougsland@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Douglas Schilling Landgraf dougsland@redhat.com Gerrit-Reviewer: Moti Asayag masayag@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com
Dan Kenigsberg has posted comments on this change.
Change subject: Adding flowID ......................................................................
Patch Set 6: Looks good to me, approved
-- To view, visit http://gerrit.ovirt.org/1221 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I6c818d7a90fea41663fd48620ac9718451a8662f Gerrit-PatchSet: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Douglas Schilling Landgraf dougsland@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Douglas Schilling Landgraf dougsland@redhat.com Gerrit-Reviewer: Moti Asayag masayag@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com
Dan Kenigsberg has posted comments on this change.
Change subject: Adding flowID ......................................................................
Patch Set 7: Verified; Looks good to me, approved
rebased
-- To view, visit http://gerrit.ovirt.org/1221 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I6c818d7a90fea41663fd48620ac9718451a8662f Gerrit-PatchSet: 7 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Douglas Schilling Landgraf dougsland@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Douglas Schilling Landgraf dougsland@redhat.com Gerrit-Reviewer: Moti Asayag masayag@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com
Dan Kenigsberg has submitted this change and it was merged.
Change subject: Adding flowID ......................................................................
Adding flowID
This patch will implement the flowID schema in vdsm side. The idea is provide a mechanism to identify the flow between ovirt engine and vdsm API calls in order to identify the entire scope of the backend action (which possibly consisted of several VSDM calls). At moment, we are going to use HTTP headers to implement this feature to keep the compability with current API, avoiding possible breakes.
Change-Id: I6c818d7a90fea41663fd48620ac9718451a8662f --- M vdsm/BindingXMLRPC.py 1 file changed, 20 insertions(+), 3 deletions(-)
Approvals: Dan Kenigsberg: Verified; Looks good to me, approved
-- To view, visit http://gerrit.ovirt.org/1221 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: merged Gerrit-Change-Id: I6c818d7a90fea41663fd48620ac9718451a8662f Gerrit-PatchSet: 7 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Douglas Schilling Landgraf dougsland@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Douglas Schilling Landgraf dougsland@redhat.com Gerrit-Reviewer: Moti Asayag masayag@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com
vdsm-patches@lists.fedorahosted.org