Re: Review Request: QPID-3222: Fix ttl overflow
--- 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
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
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
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-subscr...@qpid.apache.org
Re: Review Request: QPID-3222: Fix ttl overflow
---
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
Re: Review Request: QPID-3222: Fix ttl overflow
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
--- 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
