You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by Kenneth Giusti <kg...@apache.org> on 2015/08/17 22:34:44 UTC
Review Request 37553: Use the monotonic clock to compute the AMQP1.0
idle timer interval
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37553/
-----------------------------------------------------------
Review request for qpid and Gordon Sim.
Bugs: qpid-6698
https://issues.apache.org/jira/browse/qpid-6698
Repository: qpid
Description
-------
Summary says it all
Diffs
-----
trunk/qpid/cpp/src/qpid/broker/amqp/Connection.cpp 1694323
Diff: https://reviews.apache.org/r/37553/diff/
Testing
-------
unit tests
reproduced via changing wall clock time directly
Thanks,
Kenneth Giusti
Re: Review Request 37553: Use the monotonic clock to compute the
AMQP1.0 idle timer interval
Posted by Kenneth Giusti <kg...@apache.org>.
> On Aug. 18, 2015, 7:49 a.m., Gordon Sim wrote:
> > Fix for the broker seems fine. Perhaps the client should have a similar change? Long running clients would have the same issue.
> >
> > I do also think that the Time.h should be modified/augmented somehow to make issues like this clearer. E.g. a Duration::FromZero() alongside the FromEpoch with clear advice as to which to use when? The AbsTime::epoch() method subtracts a FromEpoch duration, computed using the realtime clock, from the now() which is computed from the monotonic clock. Is that even valid? These are arguably separate issues though, just raising tnem for debate.
>
> Kenneth Giusti wrote:
> Thanks. I'll investigate the client side and make a similar change. I'll check these simple changes in just to address the JIRA.
>
> I certainly agree the Time interface needs clarification as you suggest - I'll open a second JIRA for that. However, the real problem is with Proton's definition of pn_transport_tick - it takes a pn_timestamp_t type as the timestamp argument, which, by definition, IS wallclock time. Since pn_timestamp_t is a int64_t, I'll open a Proton JIRA to change the timestamp arguments type and description, update the docs and any examples to use a mono clock.
I've captured both points raised by Gordon in the following JIRAs:
https://issues.apache.org/jira/browse/QPID-6699
https://issues.apache.org/jira/browse/PROTON-985
- Kenneth
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37553/#review95695
-----------------------------------------------------------
On Aug. 17, 2015, 8:34 p.m., Kenneth Giusti wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37553/
> -----------------------------------------------------------
>
> (Updated Aug. 17, 2015, 8:34 p.m.)
>
>
> Review request for qpid and Gordon Sim.
>
>
> Bugs: qpid-6698
> https://issues.apache.org/jira/browse/qpid-6698
>
>
> Repository: qpid
>
>
> Description
> -------
>
> Summary says it all
>
>
> Diffs
> -----
>
> trunk/qpid/cpp/src/qpid/broker/amqp/Connection.cpp 1694323
>
> Diff: https://reviews.apache.org/r/37553/diff/
>
>
> Testing
> -------
>
> unit tests
> reproduced via changing wall clock time directly
>
>
> Thanks,
>
> Kenneth Giusti
>
>
Re: Review Request 37553: Use the monotonic clock to compute the
AMQP1.0 idle timer interval
Posted by Kenneth Giusti <kg...@apache.org>.
> On Aug. 18, 2015, 7:49 a.m., Gordon Sim wrote:
> > Fix for the broker seems fine. Perhaps the client should have a similar change? Long running clients would have the same issue.
> >
> > I do also think that the Time.h should be modified/augmented somehow to make issues like this clearer. E.g. a Duration::FromZero() alongside the FromEpoch with clear advice as to which to use when? The AbsTime::epoch() method subtracts a FromEpoch duration, computed using the realtime clock, from the now() which is computed from the monotonic clock. Is that even valid? These are arguably separate issues though, just raising tnem for debate.
Thanks. I'll investigate the client side and make a similar change. I'll check these simple changes in just to address the JIRA.
I certainly agree the Time interface needs clarification as you suggest - I'll open a second JIRA for that. However, the real problem is with Proton's definition of pn_transport_tick - it takes a pn_timestamp_t type as the timestamp argument, which, by definition, IS wallclock time. Since pn_timestamp_t is a int64_t, I'll open a Proton JIRA to change the timestamp arguments type and description, update the docs and any examples to use a mono clock.
- Kenneth
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37553/#review95695
-----------------------------------------------------------
On Aug. 17, 2015, 8:34 p.m., Kenneth Giusti wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37553/
> -----------------------------------------------------------
>
> (Updated Aug. 17, 2015, 8:34 p.m.)
>
>
> Review request for qpid and Gordon Sim.
>
>
> Bugs: qpid-6698
> https://issues.apache.org/jira/browse/qpid-6698
>
>
> Repository: qpid
>
>
> Description
> -------
>
> Summary says it all
>
>
> Diffs
> -----
>
> trunk/qpid/cpp/src/qpid/broker/amqp/Connection.cpp 1694323
>
> Diff: https://reviews.apache.org/r/37553/diff/
>
>
> Testing
> -------
>
> unit tests
> reproduced via changing wall clock time directly
>
>
> Thanks,
>
> Kenneth Giusti
>
>
Re: Review Request 37553: Use the monotonic clock to compute the
AMQP1.0 idle timer interval
Posted by Gordon Sim <gs...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37553/#review95695
-----------------------------------------------------------
Ship it!
Fix for the broker seems fine. Perhaps the client should have a similar change? Long running clients would have the same issue.
I do also think that the Time.h should be modified/augmented somehow to make issues like this clearer. E.g. a Duration::FromZero() alongside the FromEpoch with clear advice as to which to use when? The AbsTime::epoch() method subtracts a FromEpoch duration, computed using the realtime clock, from the now() which is computed from the monotonic clock. Is that even valid? These are arguably separate issues though, just raising tnem for debate.
- Gordon Sim
On Aug. 17, 2015, 8:34 p.m., Kenneth Giusti wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37553/
> -----------------------------------------------------------
>
> (Updated Aug. 17, 2015, 8:34 p.m.)
>
>
> Review request for qpid and Gordon Sim.
>
>
> Bugs: qpid-6698
> https://issues.apache.org/jira/browse/qpid-6698
>
>
> Repository: qpid
>
>
> Description
> -------
>
> Summary says it all
>
>
> Diffs
> -----
>
> trunk/qpid/cpp/src/qpid/broker/amqp/Connection.cpp 1694323
>
> Diff: https://reviews.apache.org/r/37553/diff/
>
>
> Testing
> -------
>
> unit tests
> reproduced via changing wall clock time directly
>
>
> Thanks,
>
> Kenneth Giusti
>
>