Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12221123:
Include version information in heartbeats. (Closed)
|
Created:
7 years, 10 months ago by Jamie Modified:
7 years, 10 months ago CC:
chromium-reviews, jamiewalch+watch_chromium.org, dcaiafa+watch_chromium.org, simonmorris+watch_chromium.org, hclam+watch_chromium.org, wez+watch_chromium.org, amit, sanjeevr, garykac+watch_chromium.org, lambroslambrou+watch_chromium.org, rmsousa+watch_chromium.org, alexeypa+watch_chromium.org, sergeyu+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionInclude version information in heartbeats.
This will allow us to propagate it to the directory so that the client-side
web-app can identify out-of-date hosts, as discussed in the referenced bug
(option 3 in Renato's comment).
BUG=173502
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=182379
Patch Set 1 #
Total comments: 9
Patch Set 2 : Use an element instead of an attribute for the host version. #
Total comments: 4
Patch Set 3 : Rebase #Patch Set 4 : Rename |query| to |heartbeat|. #Messages
Total messages: 19 (0 generated)
PTAL. Note that the server-side code to process the version number is not yet in place, but I would like to land the host-side CL before the branch-point if possible. My understanding is that the unrecognized elements in the heartbeat stanzas are not a problem (and testing seems to confirm this).
https://codereview.chromium.org/12221123/diff/1/remoting/host/heartbeat_sende... File remoting/host/heartbeat_sender.cc (right): https://codereview.chromium.org/12221123/diff/1/remoting/host/heartbeat_sende... remoting/host/heartbeat_sender.cc:246: query->AddAttr(QName(kChromotingXmlNamespace, kHostVersionAttr), nit: Does this need to be an attribute? It may be better add it as a separate element, so the heartbeat looks as follows: <heartbeat hostid=".." sequence-id="123"> <signature>..Signature..</signature> <host-version>..version..</host-version> </heartbeat> https://codereview.chromium.org/12221123/diff/1/remoting/host/heartbeat_sende... remoting/host/heartbeat_sender.cc:250: scoped_ptr<XmlElement> log(ServerLogEntry::MakeStanza()); The log entry should already include information about the host version. Can we just reuse it instead of duplicating it in the same message?
https://codereview.chromium.org/12221123/diff/1/remoting/host/heartbeat_sende... File remoting/host/heartbeat_sender.cc (right): https://codereview.chromium.org/12221123/diff/1/remoting/host/heartbeat_sende... remoting/host/heartbeat_sender.cc:246: query->AddAttr(QName(kChromotingXmlNamespace, kHostVersionAttr), On 2013/02/11 20:29:11, sergeyu wrote: > nit: Does this need to be an attribute? It may be better add it as a separate > element, so the heartbeat looks as follows: > <heartbeat hostid=".." sequence-id="123"> > <signature>..Signature..</signature> > <host-version>..version..</host-version> > </heartbeat> Either works for me. I'd be interested to know what the pros and cons are of your proposed change though. https://codereview.chromium.org/12221123/diff/1/remoting/host/heartbeat_sende... remoting/host/heartbeat_sender.cc:250: scoped_ptr<XmlElement> log(ServerLogEntry::MakeStanza()); On 2013/02/11 20:29:11, sergeyu wrote: > The log entry should already include information about the host version. Can we > just reuse it instead of duplicating it in the same message? Renato suggested that in the bug discussion, but described it as a hack. You guys know the server side a lot better than I do, so I'll defer to your judgement. One advantage of that approach is that this CL isn't needed and we have the information we need for older hosts. Renato, do you agree with Sergey's preference to reuse the information from the log stanza?
https://codereview.chromium.org/12221123/diff/1/remoting/host/heartbeat_sende... File remoting/host/heartbeat_sender.cc (right): https://codereview.chromium.org/12221123/diff/1/remoting/host/heartbeat_sende... remoting/host/heartbeat_sender.cc:246: query->AddAttr(QName(kChromotingXmlNamespace, kHostVersionAttr), On 2013/02/11 20:41:25, Jamie wrote: > On 2013/02/11 20:29:11, sergeyu wrote: > > nit: Does this need to be an attribute? It may be better add it as a separate > > element, so the heartbeat looks as follows: > > <heartbeat hostid=".." sequence-id="123"> > > <signature>..Signature..</signature> > > <host-version>..version..</host-version> > > </heartbeat> > > Either works for me. I'd be interested to know what the pros and cons are of > your proposed change though. There are no hard rules about it, but in general elements should be preferred, particularly because they are easier to extend. In this case putting host-version in an element would be better because in the future we may want to add new fields, e.g. we may need to include OS version. Here is a good article on the topic of attributes vs elements in XML: http://www.ibm.com/developerworks/xml/library/x-eleatt/index.html E.g. it says "- If the information in question could be itself marked up with elements, put it in an element." https://codereview.chromium.org/12221123/diff/1/remoting/host/heartbeat_sende... remoting/host/heartbeat_sender.cc:250: scoped_ptr<XmlElement> log(ServerLogEntry::MakeStanza()); On 2013/02/11 20:41:25, Jamie wrote: > On 2013/02/11 20:29:11, sergeyu wrote: > > The log entry should already include information about the host version. Can > we > > just reuse it instead of duplicating it in the same message? > > Renato suggested that in the bug discussion, but described it as a hack. You > guys know the server side a lot better than I do, so I'll defer to your > judgement. One advantage of that approach is that this CL isn't needed and we > have the information we need for older hosts. > > Renato, do you agree with Sergey's preference to reuse the information from the > log stanza? Yes. I agree. Given that we want to store this information in the directory, reusing logs would be a hack.
ptal https://codereview.chromium.org/12221123/diff/1/remoting/host/heartbeat_sende... File remoting/host/heartbeat_sender.cc (right): https://codereview.chromium.org/12221123/diff/1/remoting/host/heartbeat_sende... remoting/host/heartbeat_sender.cc:246: query->AddAttr(QName(kChromotingXmlNamespace, kHostVersionAttr), On 2013/02/11 21:40:49, sergeyu wrote: > On 2013/02/11 20:41:25, Jamie wrote: > > On 2013/02/11 20:29:11, sergeyu wrote: > > > nit: Does this need to be an attribute? It may be better add it as a > separate > > > element, so the heartbeat looks as follows: > > > <heartbeat hostid=".." sequence-id="123"> > > > <signature>..Signature..</signature> > > > <host-version>..version..</host-version> > > > </heartbeat> > > > > Either works for me. I'd be interested to know what the pros and cons are of > > your proposed change though. > > There are no hard rules about it, but in general elements should be preferred, > particularly because they are easier to extend. In this case putting > host-version in an element would be better because in the future we may want to > add new fields, e.g. we may need to include OS version. > > Here is a good article on the topic of attributes vs elements in XML: > http://www.ibm.com/developerworks/xml/library/x-eleatt/index.html > E.g. it says > "- If the information in question could be itself marked up with elements, put > it in an element." Thanks for the summary. Done. https://codereview.chromium.org/12221123/diff/1/remoting/host/heartbeat_sende... remoting/host/heartbeat_sender.cc:250: scoped_ptr<XmlElement> log(ServerLogEntry::MakeStanza()); On 2013/02/11 21:40:49, sergeyu wrote: > On 2013/02/11 20:41:25, Jamie wrote: > > On 2013/02/11 20:29:11, sergeyu wrote: > > > The log entry should already include information about the host version. Can > > we > > > just reuse it instead of duplicating it in the same message? > > > > Renato suggested that in the bug discussion, but described it as a hack. You > > guys know the server side a lot better than I do, so I'll defer to your > > judgement. One advantage of that approach is that this CL isn't needed and we > > have the information we need for older hosts. > > > > Renato, do you agree with Sergey's preference to reuse the information from > the > > log stanza? > > Yes. I agree. Given that we want to store this information in the directory, > reusing logs would be a hack. So it sounds like you're in favour of the approach taken in this CL, correct?
https://codereview.chromium.org/12221123/diff/5001/remoting/host/heartbeat_se... File remoting/host/heartbeat_sender.cc (right): https://codereview.chromium.org/12221123/diff/5001/remoting/host/heartbeat_se... remoting/host/heartbeat_sender.cc:251: query->AddElement(version_tag.release()); Actually this should be a child of <heartbeat> element. XMPP spec requires that set <iq> stanza has only one child. From the spec: An IQ stanza of type "get" or "set" MUST contain exactly one child element, which specifies the semantics of the particular request. http://xmpp.org/rfcs/rfc6120.html#stanzas-semantics-iq I understand that we are already violating this rule with the log message below, but lets not make the problem worse.
https://codereview.chromium.org/12221123/diff/1/remoting/host/heartbeat_sende... File remoting/host/heartbeat_sender.cc (right): https://codereview.chromium.org/12221123/diff/1/remoting/host/heartbeat_sende... remoting/host/heartbeat_sender.cc:250: scoped_ptr<XmlElement> log(ServerLogEntry::MakeStanza()); On 2013/02/13 18:25:26, Jamie wrote: > On 2013/02/11 21:40:49, sergeyu wrote: > > On 2013/02/11 20:41:25, Jamie wrote: > > > On 2013/02/11 20:29:11, sergeyu wrote: > > > > The log entry should already include information about the host version. > Can > > > we > > > > just reuse it instead of duplicating it in the same message? > > > > > > Renato suggested that in the bug discussion, but described it as a hack. You > > > guys know the server side a lot better than I do, so I'll defer to your > > > judgement. One advantage of that approach is that this CL isn't needed and > we > > > have the information we need for older hosts. > > > > > > Renato, do you agree with Sergey's preference to reuse the information from > > the > > > log stanza? > > > > Yes. I agree. Given that we want to store this information in the directory, > > reusing logs would be a hack. > > So it sounds like you're in favour of the approach taken in this CL, correct? Yes, I'm in favor of this approach. Sorry for not being clear.
https://codereview.chromium.org/12221123/diff/5001/remoting/host/heartbeat_se... File remoting/host/heartbeat_sender.cc (right): https://codereview.chromium.org/12221123/diff/5001/remoting/host/heartbeat_se... remoting/host/heartbeat_sender.cc:251: query->AddElement(version_tag.release()); On 2013/02/13 18:46:44, sergeyu wrote: > Actually this should be a child of <heartbeat> element. XMPP spec requires that > set <iq> stanza has only one child. From the spec: > An IQ stanza of type "get" or "set" MUST contain exactly one > child element, which specifies the semantics of the particular > request. > > http://xmpp.org/rfcs/rfc6120.html#stanzas-semantics-iq > > I understand that we are already violating this rule with the log message below, > but lets not make the problem worse. I think both the host version and the log are already children of the hearbeat element. |query| is defined using kHeartbeatQueryTag.
lgtm https://codereview.chromium.org/12221123/diff/5001/remoting/host/heartbeat_se... File remoting/host/heartbeat_sender.cc (right): https://codereview.chromium.org/12221123/diff/5001/remoting/host/heartbeat_se... remoting/host/heartbeat_sender.cc:251: query->AddElement(version_tag.release()); On 2013/02/13 19:04:38, Jamie wrote: > On 2013/02/13 18:46:44, sergeyu wrote: > > Actually this should be a child of <heartbeat> element. XMPP spec requires > that > > set <iq> stanza has only one child. From the spec: > > An IQ stanza of type "get" or "set" MUST contain exactly one > > child element, which specifies the semantics of the particular > > request. > > > > http://xmpp.org/rfcs/rfc6120.html#stanzas-semantics-iq > > > > I understand that we are already violating this rule with the log message > below, > > but lets not make the problem worse. > > I think both the host version and the log are already children of the hearbeat > element. |query| is defined using kHeartbeatQueryTag. Ah, sorry, I misread this code. |query| is not the best name for this variable.
fyi https://codereview.chromium.org/12221123/diff/5001/remoting/host/heartbeat_se... File remoting/host/heartbeat_sender.cc (right): https://codereview.chromium.org/12221123/diff/5001/remoting/host/heartbeat_se... remoting/host/heartbeat_sender.cc:251: query->AddElement(version_tag.release()); On 2013/02/13 19:23:57, sergeyu wrote: > On 2013/02/13 19:04:38, Jamie wrote: > > On 2013/02/13 18:46:44, sergeyu wrote: > > > Actually this should be a child of <heartbeat> element. XMPP spec requires > > that > > > set <iq> stanza has only one child. From the spec: > > > An IQ stanza of type "get" or "set" MUST contain exactly one > > > child element, which specifies the semantics of the particular > > > request. > > > > > > http://xmpp.org/rfcs/rfc6120.html#stanzas-semantics-iq > > > > > > I understand that we are already violating this rule with the log message > > below, > > > but lets not make the problem worse. > > > > I think both the host version and the log are already children of the hearbeat > > element. |query| is defined using kHeartbeatQueryTag. > > Ah, sorry, I misread this code. |query| is not the best name for this variable. Agreed. I've changed it to |heartbeat|.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jamiewalch@chromium.org/12221123/16001
Retried try job too often on win_rel for step(s) interactive_ui_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jamiewalch@chromium.org/12221123/16001
Retried try job too often on linux_chromeos for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jamiewalch@chromium.org/12221123/16001
Retried try job too often on linux_chromeos for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jamiewalch@chromium.org/12221123/16001
Message was sent while issue was closed.
Change committed as 182379 |
