You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by Gordon Sim <gs...@redhat.com> on 2011/04/20 18:47:00 UTC
Review Request: QPID-3222: Fix ttl overflow
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/627/
-----------------------------------------------------------
Review request for qpid, Andrew Stitcher, Alan Conway, Chug Rolke, and Steve Huston.
Summary
-------
Fixes ttl overflow on the broker. Added equality operator for qpid::messaging::Duration (needed it in test and seemed generally valuable).
This addresses bug QPID-3222.
https://issues.apache.org/jira/browse/QPID-3222
Diffs
-----
/trunk/qpid/cpp/include/qpid/messaging/Duration.h 1090157
/trunk/qpid/cpp/src/qpid/broker/Message.cpp 1090157
/trunk/qpid/cpp/src/qpid/messaging/Duration.cpp 1090157
/trunk/qpid/cpp/src/tests/MessagingSessionTests.cpp 1090157
Diff: https://reviews.apache.org/r/627/diff
Testing
-------
New test added, make check passes.
Thanks,
Gordon
Re: Review Request: QPID-3222: Fix ttl overflow
Posted by Chug Rolke <cr...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/627/#review510
-----------------------------------------------------------
Ship it!
Agreed. I'll patch the .NET file if you don't.
- Chug
On 2011-04-20 16:47:00, Gordon Sim wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/627/
> -----------------------------------------------------------
>
> (Updated 2011-04-20 16:47:00)
>
>
> Review request for qpid, Andrew Stitcher, Alan Conway, Chug Rolke, and Steve Huston.
>
>
> Summary
> -------
>
> Fixes ttl overflow on the broker. Added equality operator for qpid::messaging::Duration (needed it in test and seemed generally valuable).
>
>
> This addresses bug QPID-3222.
> https://issues.apache.org/jira/browse/QPID-3222
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/include/qpid/messaging/Duration.h 1090157
> /trunk/qpid/cpp/src/qpid/broker/Message.cpp 1090157
> /trunk/qpid/cpp/src/qpid/messaging/Duration.cpp 1090157
> /trunk/qpid/cpp/src/tests/MessagingSessionTests.cpp 1090157
>
> Diff: https://reviews.apache.org/r/627/diff
>
>
> Testing
> -------
>
> New test added, make check passes.
>
>
> Thanks,
>
> Gordon
>
>
Re: Review Request: QPID-3222: Fix ttl overflow
Posted by Alan Conway <ac...@redhat.com>.
On 04/20/2011 01:14 PM, Gordon Sim wrote:
>
>
>> On 2011-04-20 16:59:07, Alan Conway wrote:
>>> The code looks OK, but do we really need to worry about people setting a TTL of more than 17 billion years?
>>
>> Alan Conway wrote:
>> Actually looking at the BZ, the real issue is giving FOREVER the correct special treatment at each step, it's not really an overflow issue at all.
>
> FOREVER is indeed the most important case. However as the value on the wire is uint64 (and is in ms), we need to make sure that no positive value that is sent is turned into a negative value and thus immediately marks the messages as expired. It is indeed very unlikely that anyone would have a real need to use anything other than FOREVER that would cause this, but other values could trigger the same thing, so why not fix the overflow entirely.
>
Agreed, it does no harm.
---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project: http://qpid.apache.org
Use/Interact: mailto:dev-subscribe@qpid.apache.org
Re: Review Request: QPID-3222: Fix ttl overflow
Posted by Gordon Sim <gs...@redhat.com>.
> On 2011-04-20 16:59:07, Alan Conway wrote:
> > The code looks OK, but do we really need to worry about people setting a TTL of more than 17 billion years?
>
> Alan Conway wrote:
> Actually looking at the BZ, the real issue is giving FOREVER the correct special treatment at each step, it's not really an overflow issue at all.
FOREVER is indeed the most important case. However as the value on the wire is uint64 (and is in ms), we need to make sure that no positive value that is sent is turned into a negative value and thus immediately marks the messages as expired. It is indeed very unlikely that anyone would have a real need to use anything other than FOREVER that would cause this, but other values could trigger the same thing, so why not fix the overflow entirely.
- Gordon
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/627/#review501
-----------------------------------------------------------
On 2011-04-20 16:47:00, Gordon Sim wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/627/
> -----------------------------------------------------------
>
> (Updated 2011-04-20 16:47:00)
>
>
> Review request for qpid, Andrew Stitcher, Alan Conway, Chug Rolke, and Steve Huston.
>
>
> Summary
> -------
>
> Fixes ttl overflow on the broker. Added equality operator for qpid::messaging::Duration (needed it in test and seemed generally valuable).
>
>
> This addresses bug QPID-3222.
> https://issues.apache.org/jira/browse/QPID-3222
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/include/qpid/messaging/Duration.h 1090157
> /trunk/qpid/cpp/src/qpid/broker/Message.cpp 1090157
> /trunk/qpid/cpp/src/qpid/messaging/Duration.cpp 1090157
> /trunk/qpid/cpp/src/tests/MessagingSessionTests.cpp 1090157
>
> Diff: https://reviews.apache.org/r/627/diff
>
>
> Testing
> -------
>
> New test added, make check passes.
>
>
> Thanks,
>
> Gordon
>
>
Re: Review Request: QPID-3222: Fix ttl overflow
Posted by Alan Conway <ac...@redhat.com>.
> On 2011-04-20 16:59:07, Alan Conway wrote:
> > The code looks OK, but do we really need to worry about people setting a TTL of more than 17 billion years?
Actually looking at the BZ, the real issue is giving FOREVER the correct special treatment at each step, it's not really an overflow issue at all.
- Alan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/627/#review501
-----------------------------------------------------------
On 2011-04-20 16:47:00, Gordon Sim wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/627/
> -----------------------------------------------------------
>
> (Updated 2011-04-20 16:47:00)
>
>
> Review request for qpid, Andrew Stitcher, Alan Conway, Chug Rolke, and Steve Huston.
>
>
> Summary
> -------
>
> Fixes ttl overflow on the broker. Added equality operator for qpid::messaging::Duration (needed it in test and seemed generally valuable).
>
>
> This addresses bug QPID-3222.
> https://issues.apache.org/jira/browse/QPID-3222
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/include/qpid/messaging/Duration.h 1090157
> /trunk/qpid/cpp/src/qpid/broker/Message.cpp 1090157
> /trunk/qpid/cpp/src/qpid/messaging/Duration.cpp 1090157
> /trunk/qpid/cpp/src/tests/MessagingSessionTests.cpp 1090157
>
> Diff: https://reviews.apache.org/r/627/diff
>
>
> Testing
> -------
>
> New test added, make check passes.
>
>
> Thanks,
>
> Gordon
>
>
Re: Review Request: QPID-3222: Fix ttl overflow
Posted by Alan Conway <ac...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/627/#review501
-----------------------------------------------------------
Ship it!
The code looks OK, but do we really need to worry about people setting a TTL of more than 17 billion years?
- Alan
On 2011-04-20 16:47:00, Gordon Sim wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/627/
> -----------------------------------------------------------
>
> (Updated 2011-04-20 16:47:00)
>
>
> Review request for qpid, Andrew Stitcher, Alan Conway, Chug Rolke, and Steve Huston.
>
>
> Summary
> -------
>
> Fixes ttl overflow on the broker. Added equality operator for qpid::messaging::Duration (needed it in test and seemed generally valuable).
>
>
> This addresses bug QPID-3222.
> https://issues.apache.org/jira/browse/QPID-3222
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/include/qpid/messaging/Duration.h 1090157
> /trunk/qpid/cpp/src/qpid/broker/Message.cpp 1090157
> /trunk/qpid/cpp/src/qpid/messaging/Duration.cpp 1090157
> /trunk/qpid/cpp/src/tests/MessagingSessionTests.cpp 1090157
>
> Diff: https://reviews.apache.org/r/627/diff
>
>
> Testing
> -------
>
> New test added, make check passes.
>
>
> Thanks,
>
> Gordon
>
>
Re: Review Request: QPID-3222: Fix ttl overflow
Posted by Gordon Sim <gs...@redhat.com>.
> On 2011-04-20 18:04:37, Chug Rolke wrote:
> > 1. To be complete the equality tests must propagate to the .NET binding as well. See patch below.
> >
> > 2. This patch changes the API/ABI a little does it not?
> > For the .NET case assume you have Duration A(100) and Duration B(100).
> > Before this patch A==B is false and after this patch A==B is true.
> > How can we sell that?
> >
> > -Chuck
> >
> >
> > Index: qpid/cpp/bindings/qpid/dotnet/src/Duration.h
> > ===================================================================
> > --- qpid/cpp/bindings/qpid/dotnet/src/Duration.h (revision 1089977)
> > +++ qpid/cpp/bindings/qpid/dotnet/src/Duration.h (working copy)
> > @@ -81,8 +81,18 @@
> > Duration ^ result = gcnew Duration(multiplier * dur->Milliseconds);
> > return result;
> > }
> > - };
> >
> > + static bool operator == (Duration ^ a, Duration ^ b)
> > + {
> > + return a->Milliseconds == b->Milliseconds;
> > + }
> > +
> > + static bool operator != (Duration ^ a, Duration ^ b)
> > + {
> > + return a->Milliseconds != b->Milliseconds;
> > + }
> > +};
> > +
> > public ref class DurationConstants sealed
> > {
> > private:
> >
As far as c++ API/ABI goes this is purely additive (it is a change but can't break anything). Prior to this change comparing two durations for equality would fail to compile due to the lack of equality operator.
I would expect though that the semantic change from a .NET perspective would not only be acceptable but actually desired, no?
- Gordon
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/627/#review506
-----------------------------------------------------------
On 2011-04-20 16:47:00, Gordon Sim wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/627/
> -----------------------------------------------------------
>
> (Updated 2011-04-20 16:47:00)
>
>
> Review request for qpid, Andrew Stitcher, Alan Conway, Chug Rolke, and Steve Huston.
>
>
> Summary
> -------
>
> Fixes ttl overflow on the broker. Added equality operator for qpid::messaging::Duration (needed it in test and seemed generally valuable).
>
>
> This addresses bug QPID-3222.
> https://issues.apache.org/jira/browse/QPID-3222
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/include/qpid/messaging/Duration.h 1090157
> /trunk/qpid/cpp/src/qpid/broker/Message.cpp 1090157
> /trunk/qpid/cpp/src/qpid/messaging/Duration.cpp 1090157
> /trunk/qpid/cpp/src/tests/MessagingSessionTests.cpp 1090157
>
> Diff: https://reviews.apache.org/r/627/diff
>
>
> Testing
> -------
>
> New test added, make check passes.
>
>
> Thanks,
>
> Gordon
>
>
Re: Review Request: QPID-3222: Fix ttl overflow
Posted by Chug Rolke <cr...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/627/#review506
-----------------------------------------------------------
1. To be complete the equality tests must propagate to the .NET binding as well. See patch below.
2. This patch changes the API/ABI a little does it not?
For the .NET case assume you have Duration A(100) and Duration B(100).
Before this patch A==B is false and after this patch A==B is true.
How can we sell that?
-Chuck
Index: qpid/cpp/bindings/qpid/dotnet/src/Duration.h
===================================================================
--- qpid/cpp/bindings/qpid/dotnet/src/Duration.h (revision 1089977)
+++ qpid/cpp/bindings/qpid/dotnet/src/Duration.h (working copy)
@@ -81,8 +81,18 @@
Duration ^ result = gcnew Duration(multiplier * dur->Milliseconds);
return result;
}
- };
+ static bool operator == (Duration ^ a, Duration ^ b)
+ {
+ return a->Milliseconds == b->Milliseconds;
+ }
+
+ static bool operator != (Duration ^ a, Duration ^ b)
+ {
+ return a->Milliseconds != b->Milliseconds;
+ }
+};
+
public ref class DurationConstants sealed
{
private:
- Chug
On 2011-04-20 16:47:00, Gordon Sim wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/627/
> -----------------------------------------------------------
>
> (Updated 2011-04-20 16:47:00)
>
>
> Review request for qpid, Andrew Stitcher, Alan Conway, Chug Rolke, and Steve Huston.
>
>
> Summary
> -------
>
> Fixes ttl overflow on the broker. Added equality operator for qpid::messaging::Duration (needed it in test and seemed generally valuable).
>
>
> This addresses bug QPID-3222.
> https://issues.apache.org/jira/browse/QPID-3222
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/include/qpid/messaging/Duration.h 1090157
> /trunk/qpid/cpp/src/qpid/broker/Message.cpp 1090157
> /trunk/qpid/cpp/src/qpid/messaging/Duration.cpp 1090157
> /trunk/qpid/cpp/src/tests/MessagingSessionTests.cpp 1090157
>
> Diff: https://reviews.apache.org/r/627/diff
>
>
> Testing
> -------
>
> New test added, make check passes.
>
>
> Thanks,
>
> Gordon
>
>