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 2012/11/12 22:27:01 UTC
Review Request: proton-c: support for AMQP 1.0 connection idle timeout
negotiation and keepalives
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8021/
-----------------------------------------------------------
Review request for qpid, Andrew Stitcher and Rafael Schloming.
Description
-------
Attached is an attempt at adding Connection idle-time-out support to Proton-c, as defined by the AMQP 1.0 spec.
Notes:
1) Allows configuration of the AMQP Connection idle timeout locally, and support for receiving the remote's configured idle timeout.
2) Will send periodic keepalive (empty) frames to satisfy remote's idle requirements, if needed.
3) Will close the connection if remote violates the locally configured idle deadline
This addresses bug proton-111.
https://issues.apache.org/jira/browse/proton-111
Diffs
-----
/proton/trunk/proton-c/bindings/python/proton.py 1408412
/proton/trunk/proton-c/include/proton/driver.h 1408412
/proton/trunk/proton-c/include/proton/engine.h 1408412
/proton/trunk/proton-c/include/proton/types.h 1408412
/proton/trunk/proton-c/src/dispatcher/dispatcher.h 1408412
/proton/trunk/proton-c/src/dispatcher/dispatcher.c 1408412
/proton/trunk/proton-c/src/driver.c 1408412
/proton/trunk/proton-c/src/engine/engine-internal.h 1408412
/proton/trunk/proton-c/src/engine/engine.c 1408412
/proton/trunk/proton-c/src/messenger.c 1408412
/proton/trunk/proton-c/src/util.h 1408412
/proton/trunk/proton-c/src/util.c 1408412
/proton/trunk/proton-j/src/main/scripts/proton.py 1408412
/proton/trunk/tests/proton_tests/engine.py 1408412
Diff: https://reviews.apache.org/r/8021/diff/
Testing
-------
Added an engine test case to verify that the timers are tracked, and the action when timers fire, but I need to add a test at the driver level, too.
Thanks,
Kenneth Giusti
Re: Review Request: proton-c: support for AMQP 1.0 connection idle timeout
negotiation and keepalives
Posted by Rafael Schloming <rh...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8021/#review13432
-----------------------------------------------------------
Ship it!
Ship It!
- Rafael Schloming
On Nov. 13, 2012, 5:02 p.m., Kenneth Giusti wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8021/
> -----------------------------------------------------------
>
> (Updated Nov. 13, 2012, 5:02 p.m.)
>
>
> Review request for qpid, Andrew Stitcher and Rafael Schloming.
>
>
> Description
> -------
>
> Attached is an attempt at adding Connection idle-time-out support to Proton-c, as defined by the AMQP 1.0 spec.
>
> Notes:
>
> 1) Allows configuration of the AMQP Connection idle timeout locally, and support for receiving the remote's configured idle timeout.
> 2) Will send periodic keepalive (empty) frames to satisfy remote's idle requirements, if needed.
> 3) Will close the connection if remote violates the locally configured idle deadline
>
>
> This addresses bug proton-111.
> https://issues.apache.org/jira/browse/proton-111
>
>
> Diffs
> -----
>
> /proton/trunk/proton-c/CMakeLists.txt 1408810
> /proton/trunk/proton-c/bindings/python/proton.py 1408810
> /proton/trunk/proton-c/include/proton/driver.h 1408810
> /proton/trunk/proton-c/include/proton/engine.h 1408810
> /proton/trunk/proton-c/include/proton/types.h 1408810
> /proton/trunk/proton-c/src/dispatcher/dispatcher.h 1408810
> /proton/trunk/proton-c/src/dispatcher/dispatcher.c 1408810
> /proton/trunk/proton-c/src/driver.c 1408810
> /proton/trunk/proton-c/src/engine/engine-internal.h 1408810
> /proton/trunk/proton-c/src/engine/engine.c 1408810
> /proton/trunk/proton-c/src/messenger.c 1408810
> /proton/trunk/proton-c/src/util.h 1408810
> /proton/trunk/proton-c/src/util.c 1408810
> /proton/trunk/proton-j/src/main/scripts/proton.py 1408810
> /proton/trunk/tests/proton_tests/engine.py 1408810
>
> Diff: https://reviews.apache.org/r/8021/diff/
>
>
> Testing
> -------
>
> Added an engine test case to verify that the timers are tracked, and the action when timers fire, but I need to add a test at the driver level, too.
>
>
> Thanks,
>
> Kenneth Giusti
>
>
Re: Review Request: proton-c: support for AMQP 1.0 connection idle timeout
negotiation and keepalives
Posted by Kenneth Giusti <kg...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8021/
-----------------------------------------------------------
(Updated Nov. 13, 2012, 5:02 p.m.)
Review request for qpid, Andrew Stitcher and Rafael Schloming.
Changes
-------
fixed a bug in pn_driver_wait_2() - incorrect timeout calculation when timeout < 0.
Description
-------
Attached is an attempt at adding Connection idle-time-out support to Proton-c, as defined by the AMQP 1.0 spec.
Notes:
1) Allows configuration of the AMQP Connection idle timeout locally, and support for receiving the remote's configured idle timeout.
2) Will send periodic keepalive (empty) frames to satisfy remote's idle requirements, if needed.
3) Will close the connection if remote violates the locally configured idle deadline
This addresses bug proton-111.
https://issues.apache.org/jira/browse/proton-111
Diffs (updated)
-----
/proton/trunk/proton-c/CMakeLists.txt 1408810
/proton/trunk/proton-c/bindings/python/proton.py 1408810
/proton/trunk/proton-c/include/proton/driver.h 1408810
/proton/trunk/proton-c/include/proton/engine.h 1408810
/proton/trunk/proton-c/include/proton/types.h 1408810
/proton/trunk/proton-c/src/dispatcher/dispatcher.h 1408810
/proton/trunk/proton-c/src/dispatcher/dispatcher.c 1408810
/proton/trunk/proton-c/src/driver.c 1408810
/proton/trunk/proton-c/src/engine/engine-internal.h 1408810
/proton/trunk/proton-c/src/engine/engine.c 1408810
/proton/trunk/proton-c/src/messenger.c 1408810
/proton/trunk/proton-c/src/util.h 1408810
/proton/trunk/proton-c/src/util.c 1408810
/proton/trunk/proton-j/src/main/scripts/proton.py 1408810
/proton/trunk/tests/proton_tests/engine.py 1408810
Diff: https://reviews.apache.org/r/8021/diff/
Testing
-------
Added an engine test case to verify that the timers are tracked, and the action when timers fire, but I need to add a test at the driver level, too.
Thanks,
Kenneth Giusti
Re: Review Request: proton-c: support for AMQP 1.0 connection idle timeout
negotiation and keepalives
Posted by Kenneth Giusti <kg...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8021/
-----------------------------------------------------------
(Updated Nov. 13, 2012, 3:08 p.m.)
Review request for qpid, Andrew Stitcher and Rafael Schloming.
Changes
-------
Updated with review feedback.
Description
-------
Attached is an attempt at adding Connection idle-time-out support to Proton-c, as defined by the AMQP 1.0 spec.
Notes:
1) Allows configuration of the AMQP Connection idle timeout locally, and support for receiving the remote's configured idle timeout.
2) Will send periodic keepalive (empty) frames to satisfy remote's idle requirements, if needed.
3) Will close the connection if remote violates the locally configured idle deadline
This addresses bug proton-111.
https://issues.apache.org/jira/browse/proton-111
Diffs (updated)
-----
/proton/trunk/proton-c/CMakeLists.txt 1408412
/proton/trunk/proton-c/bindings/python/proton.py 1408412
/proton/trunk/proton-c/include/proton/driver.h 1408412
/proton/trunk/proton-c/include/proton/engine.h 1408412
/proton/trunk/proton-c/include/proton/types.h 1408412
/proton/trunk/proton-c/src/dispatcher/dispatcher.h 1408412
/proton/trunk/proton-c/src/dispatcher/dispatcher.c 1408412
/proton/trunk/proton-c/src/driver.c 1408412
/proton/trunk/proton-c/src/engine/engine-internal.h 1408412
/proton/trunk/proton-c/src/engine/engine.c 1408412
/proton/trunk/proton-c/src/messenger.c 1408412
/proton/trunk/proton-c/src/util.h 1408412
/proton/trunk/proton-c/src/util.c 1408412
/proton/trunk/proton-j/src/main/scripts/proton.py 1408412
/proton/trunk/tests/proton_tests/engine.py 1408412
Diff: https://reviews.apache.org/r/8021/diff/
Testing
-------
Added an engine test case to verify that the timers are tracked, and the action when timers fire, but I need to add a test at the driver level, too.
Thanks,
Kenneth Giusti
Re: Review Request: proton-c: support for AMQP 1.0 connection idle timeout
negotiation and keepalives
Posted by Kenneth Giusti <kg...@apache.org>.
> On Nov. 12, 2012, 10:32 p.m., Andrew Stitcher wrote:
> > /proton/trunk/proton-c/include/proton/engine.h, line 288
> > <https://reviews.apache.org/r/8021/diff/1/?file=188827#file188827line288>
> >
> > Do you really mean prior? surely if the engine is invoked prior to the deadline it will decide to do nothing since no timers have expired.
> >
> > I think you mean "as soon after the deadline as possible"
My wording bad english, not goodly written down.
;D
In practice, the driver should be invoked before that timeout, given that I/O events could be pending regardless of the timeout. Technically, if the driver is not invoked until _after_ the timeout, timers will expire and possibly lead to a connection drop (e.g. keepalive not sent in time).
A more accurate comment should probably read "invoked again at least once at or before the timeout expires".
> On Nov. 12, 2012, 10:32 p.m., Andrew Stitcher wrote:
> > /proton/trunk/proton-c/src/driver.c, line 843
> > <https://reviews.apache.org/r/8021/diff/1/?file=188831#file188831line843>
> >
> > gettimeofday() is obsolete (read man 2 gettimeofday) use clock_gettime in preference (although this then requires -lrt, but that's not a bad thing in itself).
Ok - the original code used "gettimeofday" in a couple of places - specifically in messenger. Seemed like that functionality was better abstracted to the driver.
I'll try clock_gettime()...
- Kenneth
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8021/#review13372
-----------------------------------------------------------
On Nov. 12, 2012, 9:27 p.m., Kenneth Giusti wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8021/
> -----------------------------------------------------------
>
> (Updated Nov. 12, 2012, 9:27 p.m.)
>
>
> Review request for qpid, Andrew Stitcher and Rafael Schloming.
>
>
> Description
> -------
>
> Attached is an attempt at adding Connection idle-time-out support to Proton-c, as defined by the AMQP 1.0 spec.
>
> Notes:
>
> 1) Allows configuration of the AMQP Connection idle timeout locally, and support for receiving the remote's configured idle timeout.
> 2) Will send periodic keepalive (empty) frames to satisfy remote's idle requirements, if needed.
> 3) Will close the connection if remote violates the locally configured idle deadline
>
>
> This addresses bug proton-111.
> https://issues.apache.org/jira/browse/proton-111
>
>
> Diffs
> -----
>
> /proton/trunk/proton-c/bindings/python/proton.py 1408412
> /proton/trunk/proton-c/include/proton/driver.h 1408412
> /proton/trunk/proton-c/include/proton/engine.h 1408412
> /proton/trunk/proton-c/include/proton/types.h 1408412
> /proton/trunk/proton-c/src/dispatcher/dispatcher.h 1408412
> /proton/trunk/proton-c/src/dispatcher/dispatcher.c 1408412
> /proton/trunk/proton-c/src/driver.c 1408412
> /proton/trunk/proton-c/src/engine/engine-internal.h 1408412
> /proton/trunk/proton-c/src/engine/engine.c 1408412
> /proton/trunk/proton-c/src/messenger.c 1408412
> /proton/trunk/proton-c/src/util.h 1408412
> /proton/trunk/proton-c/src/util.c 1408412
> /proton/trunk/proton-j/src/main/scripts/proton.py 1408412
> /proton/trunk/tests/proton_tests/engine.py 1408412
>
> Diff: https://reviews.apache.org/r/8021/diff/
>
>
> Testing
> -------
>
> Added an engine test case to verify that the timers are tracked, and the action when timers fire, but I need to add a test at the driver level, too.
>
>
> Thanks,
>
> Kenneth Giusti
>
>
Re: Review Request: proton-c: support for AMQP 1.0 connection idle timeout
negotiation and keepalives
Posted by Andrew Stitcher <as...@apache.org>.
> On Nov. 12, 2012, 10:32 p.m., Andrew Stitcher wrote:
> > /proton/trunk/proton-c/src/driver.c, line 843
> > <https://reviews.apache.org/r/8021/diff/1/?file=188831#file188831line843>
> >
> > gettimeofday() is obsolete (read man 2 gettimeofday) use clock_gettime in preference (although this then requires -lrt, but that's not a bad thing in itself).
>
> Kenneth Giusti wrote:
> Ok - the original code used "gettimeofday" in a couple of places - specifically in messenger. Seemed like that functionality was better abstracted to the driver.
>
> I'll try clock_gettime()...
As the both gettimeofday() and clock_gettime() are not ANSI and are POSIX platform dependent they should definitely be put in the driver not in messenger, which should ideally be ANSI as there's no real reason for it to contain platform dependent code itself.
- Andrew
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8021/#review13372
-----------------------------------------------------------
On Nov. 13, 2012, 3:08 p.m., Kenneth Giusti wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8021/
> -----------------------------------------------------------
>
> (Updated Nov. 13, 2012, 3:08 p.m.)
>
>
> Review request for qpid, Andrew Stitcher and Rafael Schloming.
>
>
> Description
> -------
>
> Attached is an attempt at adding Connection idle-time-out support to Proton-c, as defined by the AMQP 1.0 spec.
>
> Notes:
>
> 1) Allows configuration of the AMQP Connection idle timeout locally, and support for receiving the remote's configured idle timeout.
> 2) Will send periodic keepalive (empty) frames to satisfy remote's idle requirements, if needed.
> 3) Will close the connection if remote violates the locally configured idle deadline
>
>
> This addresses bug proton-111.
> https://issues.apache.org/jira/browse/proton-111
>
>
> Diffs
> -----
>
> /proton/trunk/proton-c/CMakeLists.txt 1408412
> /proton/trunk/proton-c/bindings/python/proton.py 1408412
> /proton/trunk/proton-c/include/proton/driver.h 1408412
> /proton/trunk/proton-c/include/proton/engine.h 1408412
> /proton/trunk/proton-c/include/proton/types.h 1408412
> /proton/trunk/proton-c/src/dispatcher/dispatcher.h 1408412
> /proton/trunk/proton-c/src/dispatcher/dispatcher.c 1408412
> /proton/trunk/proton-c/src/driver.c 1408412
> /proton/trunk/proton-c/src/engine/engine-internal.h 1408412
> /proton/trunk/proton-c/src/engine/engine.c 1408412
> /proton/trunk/proton-c/src/messenger.c 1408412
> /proton/trunk/proton-c/src/util.h 1408412
> /proton/trunk/proton-c/src/util.c 1408412
> /proton/trunk/proton-j/src/main/scripts/proton.py 1408412
> /proton/trunk/tests/proton_tests/engine.py 1408412
>
> Diff: https://reviews.apache.org/r/8021/diff/
>
>
> Testing
> -------
>
> Added an engine test case to verify that the timers are tracked, and the action when timers fire, but I need to add a test at the driver level, too.
>
>
> Thanks,
>
> Kenneth Giusti
>
>
Re: Review Request: proton-c: support for AMQP 1.0 connection idle timeout
negotiation and keepalives
Posted by Andrew Stitcher <as...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8021/#review13372
-----------------------------------------------------------
/proton/trunk/proton-c/include/proton/engine.h
<https://reviews.apache.org/r/8021/#comment28646>
Do you really mean prior? surely if the engine is invoked prior to the deadline it will decide to do nothing since no timers have expired.
I think you mean "as soon after the deadline as possible"
/proton/trunk/proton-c/src/driver.c
<https://reviews.apache.org/r/8021/#comment28647>
gettimeofday() is obsolete (read man 2 gettimeofday) use clock_gettime in preference (although this then requires -lrt, but that's not a bad thing in itself).
- Andrew Stitcher
On Nov. 12, 2012, 9:27 p.m., Kenneth Giusti wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8021/
> -----------------------------------------------------------
>
> (Updated Nov. 12, 2012, 9:27 p.m.)
>
>
> Review request for qpid, Andrew Stitcher and Rafael Schloming.
>
>
> Description
> -------
>
> Attached is an attempt at adding Connection idle-time-out support to Proton-c, as defined by the AMQP 1.0 spec.
>
> Notes:
>
> 1) Allows configuration of the AMQP Connection idle timeout locally, and support for receiving the remote's configured idle timeout.
> 2) Will send periodic keepalive (empty) frames to satisfy remote's idle requirements, if needed.
> 3) Will close the connection if remote violates the locally configured idle deadline
>
>
> This addresses bug proton-111.
> https://issues.apache.org/jira/browse/proton-111
>
>
> Diffs
> -----
>
> /proton/trunk/proton-c/bindings/python/proton.py 1408412
> /proton/trunk/proton-c/include/proton/driver.h 1408412
> /proton/trunk/proton-c/include/proton/engine.h 1408412
> /proton/trunk/proton-c/include/proton/types.h 1408412
> /proton/trunk/proton-c/src/dispatcher/dispatcher.h 1408412
> /proton/trunk/proton-c/src/dispatcher/dispatcher.c 1408412
> /proton/trunk/proton-c/src/driver.c 1408412
> /proton/trunk/proton-c/src/engine/engine-internal.h 1408412
> /proton/trunk/proton-c/src/engine/engine.c 1408412
> /proton/trunk/proton-c/src/messenger.c 1408412
> /proton/trunk/proton-c/src/util.h 1408412
> /proton/trunk/proton-c/src/util.c 1408412
> /proton/trunk/proton-j/src/main/scripts/proton.py 1408412
> /proton/trunk/tests/proton_tests/engine.py 1408412
>
> Diff: https://reviews.apache.org/r/8021/diff/
>
>
> Testing
> -------
>
> Added an engine test case to verify that the timers are tracked, and the action when timers fire, but I need to add a test at the driver level, too.
>
>
> Thanks,
>
> Kenneth Giusti
>
>