----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25418/#review52590 -----------------------------------------------------------
Ship it! It is a change, so any script or application that relied on directly identifying the session name would be impacted. The 'ideal' fix might have been to add a separate field, as part of the indexed values. However that would also be backward incompatible (and possibly in a more awkward way). The only way I think to fix this without breaking anything reliant on the old behaviour, would be to only modify the session name to include the userid if there was already a session of the same name. However I'm not sure this is worth the effort really (or the extra mess to the code). It also then leads to a more odd and unpredicatble naming scheme. I think we can justify potential breakage as necessary to properly fix a bug. - Gordon Sim On Sept. 7, 2014, 12:38 p.m., Pavel Moravec wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/25418/ > ----------------------------------------------------------- > > (Updated Sept. 7, 2014, 12:38 p.m.) > > > Review request for qpid, Gordon Sim and Ted Ross. > > > Bugs: QPID-6087 > https://issues.apache.org/jira/browse/QPID-6087 > > > Repository: qpid > > > Description > ------- > > Trivial fix to have QMF session object name with user ID included. > > Session names will be like: > > > org.apache.qpid.broker:session:anonymous@QPID.eab7f84c-3903-4a03-b3b0-4e8f7ab85b86:0 > org.apache.qpid.broker:session:us...@qpid.aaa > org.apache.qpid.broker:session:us...@qpid.aaa > > (the later two are for users specifying session name as "aaa"). > > Patch itself is trivial, my only concern is if it can't break some naming > convention, backward compatibility or so. > > Another approach would be to put there pointer instead of name, like AMQP 1.0 > does: > > Index: cpp/src/qpid/broker/SessionState.cpp > =================================================================== > --- cpp/src/qpid/broker/SessionState.cpp (revision 1622984) > +++ cpp/src/qpid/broker/SessionState.cpp (working copy) > @@ -69,7 +70,9 @@ > if (parent != 0) { > ManagementAgent* agent = getBroker().getManagementAgent(); > if (agent != 0) { > - std::string name(getId().getName()); > + std::stringstream ss; > + ss << static_cast<const void*>(&(getId())); > + std::string name(ss.str()); > std::string fullName(name); > if (name.length() >= std::numeric_limits<uint8_t>::max()) > name.resize(std::numeric_limits<uint8_t>::max()-1); > > This would align AMQP 0-10 session "names" in QMF to 1.0 names, but we would > loose the information about the real session name one can set in 0-10. > > > Diffs > ----- > > trunk/qpid/cpp/src/qpid/broker/SessionState.cpp 1622984 > > Diff: https://reviews.apache.org/r/25418/diff/ > > > Testing > ------- > > > Thanks, > > Pavel Moravec > >
