Re: Review Request 25100: Provide access to the Proton version numbers (major, minor) via the bindings (python only atm)
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25100/#review51657 --- Looks fine to me. I'll add similar charges to Perl and Ruby as well. - Darryl Pierce On Aug. 27, 2014, 3:31 p.m., Kenneth Giusti wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25100/ --- (Updated Aug. 27, 2014, 3:31 p.m.) Review request for qpid, Justin Ross, Darryl Pierce, Rafael Schloming, and Robbie Gemmell. Bugs: proton-651 https://issues.apache.org/jira/browse/proton-651 Repository: qpid Description --- Simple export of the version #'s. I did have to add these to the Java side, not sure how you feel about that. I'm not comfortable updating the other bindings - don't think I can make the best choice as to what feels 'natural' for the other bindings. Diffs - proton/trunk/proton-c/bindings/python/proton.py 1620643 proton/trunk/proton-c/include/proton/cproton.i 1620643 proton/trunk/proton-c/include/proton/version.h.in 1620643 proton/trunk/proton-j/src/main/java/org/apache/qpid/proton/Proton.java 1620643 proton/trunk/proton-j/src/main/resources/cproton.py 1620643 proton/trunk/proton-j/src/main/resources/cversion.py PRE-CREATION Diff: https://reviews.apache.org/r/25100/diff/ Testing --- existing unit tests pass. Verified that the proton.VERSION_MAJOR/MINOR values are correct by hand. Thanks, Kenneth Giusti
Re: Review Request 25100: Provide access to the Proton version numbers (major, minor) via the bindings (python only atm)
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25100/#review51658 --- Ship it! Ship It! - Darryl Pierce On Aug. 27, 2014, 3:31 p.m., Kenneth Giusti wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25100/ --- (Updated Aug. 27, 2014, 3:31 p.m.) Review request for qpid, Justin Ross, Darryl Pierce, Rafael Schloming, and Robbie Gemmell. Bugs: proton-651 https://issues.apache.org/jira/browse/proton-651 Repository: qpid Description --- Simple export of the version #'s. I did have to add these to the Java side, not sure how you feel about that. I'm not comfortable updating the other bindings - don't think I can make the best choice as to what feels 'natural' for the other bindings. Diffs - proton/trunk/proton-c/bindings/python/proton.py 1620643 proton/trunk/proton-c/include/proton/cproton.i 1620643 proton/trunk/proton-c/include/proton/version.h.in 1620643 proton/trunk/proton-j/src/main/java/org/apache/qpid/proton/Proton.java 1620643 proton/trunk/proton-j/src/main/resources/cproton.py 1620643 proton/trunk/proton-j/src/main/resources/cversion.py PRE-CREATION Diff: https://reviews.apache.org/r/25100/diff/ Testing --- existing unit tests pass. Verified that the proton.VERSION_MAJOR/MINOR values are correct by hand. Thanks, Kenneth Giusti
Re: Review Request 23130: Removing wrapped types from proton swigged bindings
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23130/#review46895 --- proton/trunk/proton-c/bindings/python/cproton.i https://reviews.apache.org/r/23130/#comment82468 These methods are used by the Ruby and Perl bindings. - Darryl Pierce On June 27, 2014, 5:06 p.m., Andrew Stitcher wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23130/ --- (Updated June 27, 2014, 5:06 p.m.) Review request for qpid and Darryl Pierce. Bugs: PROTON-620 https://issues.apache.org/jira/browse/PROTON-620 Repository: qpid Description --- see PROTON-620 for rationale. Note that this also has a bug fix which stops python crashing when trying to typemap non strings to pn_uuid_t Diffs - proton/trunk/proton-c/bindings/python/cproton.i 1606140 proton/trunk/proton-c/bindings/ruby/ruby.i 1606140 proton/trunk/proton-c/include/proton/cproton.i 1606140 Diff: https://reviews.apache.org/r/23130/diff/ Testing --- ctest succeeds for all languages that have tests. Thanks, Andrew Stitcher
Re: Review Request 23130: Removing wrapped types from proton swigged bindings
On June 27, 2014, 7:10 p.m., Darryl Pierce wrote: proton/trunk/proton-c/bindings/python/cproton.i, line 40 https://reviews.apache.org/r/23130/diff/1/?file=619420#file619420line40 These methods are used by the Ruby and Perl bindings. Andrew Stitcher wrote: I know - that's why I only removed them from the python binding! Sorry, I should have phrased that better. Ruby and Perl use them, should they not? I'd like to keep some parity across the bindings, so if they're not needed in Python are they still needed in other languages? - Darryl --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23130/#review46895 --- On June 27, 2014, 5:06 p.m., Andrew Stitcher wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23130/ --- (Updated June 27, 2014, 5:06 p.m.) Review request for qpid and Darryl Pierce. Bugs: PROTON-620 https://issues.apache.org/jira/browse/PROTON-620 Repository: qpid Description --- see PROTON-620 for rationale. Note that this also has a bug fix which stops python crashing when trying to typemap non strings to pn_uuid_t Diffs - proton/trunk/proton-c/bindings/python/cproton.i 1606140 proton/trunk/proton-c/bindings/ruby/ruby.i 1606140 proton/trunk/proton-c/include/proton/cproton.i 1606140 Diff: https://reviews.apache.org/r/23130/diff/ Testing --- ctest succeeds for all languages that have tests. Thanks, Andrew Stitcher
Re: Review Request 23130: Removing wrapped types from proton swigged bindings
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23130/#review46903 --- Ship it! Ship It! - Darryl Pierce On June 27, 2014, 5:06 p.m., Andrew Stitcher wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23130/ --- (Updated June 27, 2014, 5:06 p.m.) Review request for qpid and Darryl Pierce. Bugs: PROTON-620 https://issues.apache.org/jira/browse/PROTON-620 Repository: qpid Description --- see PROTON-620 for rationale. Note that this also has a bug fix which stops python crashing when trying to typemap non strings to pn_uuid_t Diffs - proton/trunk/proton-c/bindings/python/cproton.i 1606140 proton/trunk/proton-c/bindings/ruby/ruby.i 1606140 proton/trunk/proton-c/include/proton/cproton.i 1606140 Diff: https://reviews.apache.org/r/23130/diff/ Testing --- ctest succeeds for all languages that have tests. Thanks, Andrew Stitcher
Re: Review Request 21929: Split apart the read-only and writable aspects of pn_bytes_t
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21929/#review44010 --- Ship it! - Darryl Pierce On May 27, 2014, 4:32 p.m., Andrew Stitcher wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21929/ --- (Updated May 27, 2014, 4:32 p.m.) Review request for qpid, Darryl Pierce, Rafael Schloming, and Ted Ross. Bugs: PROTON-429 https://issues.apache.org/jira/browse/PROTON-429 Repository: qpid Description --- Added a new type pn_wbytes_t and added a new buffer method pn_buffer_wbytes() to return a writeable area of a buffer. This is what is necessary to make the pn_bytes_t struct read-only. pn_wbytes_t isonly used internally so doesn't need to be exposed in the API. Points/Questions: 1. It looks like pn_buffer_t itself is a purely internal API - perhaps it shouldn't be in the exported include files and doesn't need the PN_EXTERN markings. 2. As the bindings have in/out typemaps for pn_bytes_t I have removed the wrapping of the pn_bytes_t struct as it doesn't seem necessary and now it could cause problems as swig no longer generates free()s dues to the const char* member I added. 3. I removed pn_bytes_dup()) because it is never used internally and it is now unnecessary because none of the calls that takes a pn_bytes_t stores it away or tries to modify the bytes - This is an API breakage and it could be added back easily as a null function. Diffs - /proton/trunk/proton-c/bindings/perl/perl.i 1596912 /proton/trunk/proton-c/bindings/php/php.i 1596912 /proton/trunk/proton-c/bindings/python/python.i 1596912 /proton/trunk/proton-c/bindings/ruby/ruby.i 1596912 /proton/trunk/proton-c/include/proton/buffer.h 1596912 /proton/trunk/proton-c/include/proton/types.h 1596912 /proton/trunk/proton-c/src/buffer.c 1596912 /proton/trunk/proton-c/src/codec/codec.c 1596912 /proton/trunk/proton-c/src/dispatcher/dispatcher.c 1596912 /proton/trunk/proton-c/src/messenger/messenger.c 1596912 /proton/trunk/proton-c/src/sasl/sasl.c 1596912 /proton/trunk/proton-c/src/types.c 1596912 Diff: https://reviews.apache.org/r/21929/diff/ Testing --- ctest Thanks, Andrew Stitcher
Re: Review Request 18649: Make qmf-gen/qmf installation optional
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18649/#review36015 --- Ship it! Looks good to me with one small change: please adjust the leady spaces for the changed line so that it lines up as before the edit. - Darryl Pierce On March 3, 2014, 7:27 p.m., Chug Rolke wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18649/ --- (Updated March 3, 2014, 7:27 p.m.) Review request for qpid and Darryl Pierce. Bugs: qpid-5589 https://issues.apache.org/jira/browse/qpid-5589 Repository: qpid Description --- Depending on where Python is installed on windows then qpid 'make install' fails. This patch 1. Makes the install optional. 2. Escapes spaces in the install path name so that installations to Program Files work. Diffs - trunk/qpid/cpp/managementgen/CMakeLists.txt 1573098 Diff: https://reviews.apache.org/r/18649/diff/ Testing --- Installation can be skipped using the option. Installation succeeds with space in python path name. Thanks, Chug Rolke
Re: Review Request 15978: Fix python binding dependencies to avoid unnecessary rebuilds
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/15978/#review29733 --- Ship it! Verified it as working. Did a build and another and didn't see anything rebuild. Touched various Swig descriptors and saw rebuilds as expected. Looks good! - Darryl Pierce On Dec. 3, 2013, 10:33 p.m., Andrew Stitcher wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/15978/ --- (Updated Dec. 3, 2013, 10:33 p.m.) Review request for qpid and Darryl Pierce. Repository: qpid Description --- Fix the binding dependencies so that we no longer keep on rebuilding the python swig bindings unnecessarily Diffs - /trunk/qpid/cpp/bindings/qmf2/python/CMakeLists.txt 1547601 /trunk/qpid/cpp/bindings/qmf2/python/cqmf2.i PRE-CREATION /trunk/qpid/cpp/bindings/qmf2/python/python.i 1547601 /trunk/qpid/cpp/bindings/qmf2/ruby/CMakeLists.txt 1547601 /trunk/qpid/cpp/bindings/qpid/python/CMakeLists.txt 1547601 /trunk/qpid/cpp/bindings/qpid/python/python.i 1547601 /trunk/qpid/cpp/bindings/qpid/python/qpid_messaging.i PRE-CREATION /trunk/qpid/cpp/bindings/qpid/ruby/CMakeLists.txt 1547601 Diff: https://reviews.apache.org/r/15978/diff/ Testing --- build; rebuild; Does not rebuild the python bindings Thanks, Andrew Stitcher
Re: Review Request 13902: Improvement to swigged pythons handling of message properties
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/13902/#review25792 --- Ship it! Ship It! - Darryl Pierce On Aug. 30, 2013, 12:05 p.m., Gordon Sim wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/13902/ --- (Updated Aug. 30, 2013, 12:05 p.m.) Review request for qpid, Darryl Pierce and Rafael Schloming. Bugs: QPID-5104 https://issues.apache.org/jira/browse/QPID-5104 Repository: qpid Description --- A present the dict return by msg.properties when using the swigged python library, will return a copy of the properties. Modifying this has no effect on the message properties as set. This proposal instead returns a custom object that looks like a dict, but on setting a value it will copy that value to the underlying c++ message object. Diffs - /trunk/qpid/cpp/bindings/qpid/python/python.i 1518236 /trunk/qpid/python/qpid/tests/messaging/message.py 1518236 Diff: https://reviews.apache.org/r/13902/diff/ Testing --- Tested spout/drain with -P option; added simple test using 'msg.properties[key] = value' pattern. Thanks, Gordon Sim
Re: Review Request 13902: Improvement to swigged pythons handling of message properties
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/13902/#review25751 --- /trunk/qpid/cpp/bindings/qpid/python/python.i https://reviews.apache.org/r/13902/#comment50269 Would it be possible to cache the instance of MessageProperties that's returned here rather than creating a new instance each tmie? - Darryl Pierce On Aug. 29, 2013, 5:40 p.m., Gordon Sim wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/13902/ --- (Updated Aug. 29, 2013, 5:40 p.m.) Review request for qpid, Darryl Pierce and Rafael Schloming. Bugs: QPID-5104 https://issues.apache.org/jira/browse/QPID-5104 Repository: qpid Description --- A present the dict return by msg.properties when using the swigged python library, will return a copy of the properties. Modifying this has no effect on the message properties as set. This proposal instead returns a custom object that looks like a dict, but on setting a value it will copy that value to the underlying c++ message object. Diffs - /trunk/qpid/cpp/bindings/qpid/python/python.i 1518236 /trunk/qpid/python/qpid/tests/messaging/message.py 1518236 Diff: https://reviews.apache.org/r/13902/diff/ Testing --- Tested spout/drain with -P option; added simple test using 'msg.properties[key] = value' pattern. Thanks, Gordon Sim
Re: Review Request 12515: Tests for the swigged python client
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/12515/#review23096 --- Ship it! Ship It! - Darryl Pierce On July 12, 2013, 3:36 p.m., Gordon Sim wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/12515/ --- (Updated July 12, 2013, 3:36 p.m.) Review request for qpid, Alan Conway, Justin Ross, Darryl Pierce, Rafael Schloming, and Ted Ross. Bugs: QPID-4988 https://issues.apache.org/jira/browse/QPID-4988 Repository: qpid Description --- Adds runs of some of the python tests over the swigged client. Also adds some new tests for AMQP 1.0. Diffs - /trunk/qpid/cpp/bindings/qpid/python/CMakeLists.txt 1502193 /trunk/qpid/cpp/bindings/qpid/python/python.i 1502193 /trunk/qpid/cpp/src/Makefile.am 1502193 /trunk/qpid/cpp/src/amqp.cmake 1502193 /trunk/qpid/cpp/src/tests/CMakeLists.txt 1502193 /trunk/qpid/cpp/src/tests/Makefile.am 1502193 /trunk/qpid/cpp/src/tests/failing-amqp0-10-python-tests PRE-CREATION /trunk/qpid/cpp/src/tests/swig_python_tests PRE-CREATION /trunk/qpid/cpp/src/tests/test_env.sh.in 1502193 /trunk/qpid/python/qpid/tests/messaging/__init__.py 1502193 /trunk/qpid/python/qpid/tests/messaging/implementation.py PRE-CREATION /trunk/qpid/python/qpid/tests/messaging/message.py 1502193 /trunk/qpid/tests/src/py/qpid_tests/broker_0_10/lvq.py 1502193 /trunk/qpid/tests/src/py/qpid_tests/broker_0_10/new_api.py 1502193 /trunk/qpid/tests/src/py/qpid_tests/broker_0_10/priority.py 1502193 /trunk/qpid/tests/src/py/qpid_tests/broker_0_10/stats.py 1502193 /trunk/qpid/tests/src/py/qpid_tests/broker_1_0/__init__.py PRE-CREATION /trunk/qpid/tests/src/py/qpid_tests/broker_1_0/general.py PRE-CREATION /trunk/qpid/tests/src/py/qpid_tests/broker_1_0/legacy_exchanges.py PRE-CREATION /trunk/qpid/tests/src/py/qpid_tests/broker_1_0/selector.py PRE-CREATION Diff: https://reviews.apache.org/r/12515/diff/ Testing --- cmake (make make test) and autotools (make check) pass Thanks, Gordon Sim
Re: Review Request: QPID-4733: Fix cmake installation of init scripts
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/10398/#review18974 --- Ship it! It should be rebased on the latest tip. But otherwise it's good to go. - Darryl Pierce On April 10, 2013, 5:33 p.m., Alan Conway wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/10398/ --- (Updated April 10, 2013, 5:33 p.m.) Review request for qpid and Darryl Pierce. Description --- QPID-4733: Fix cmake installation of init scripts - Install both qpidd and qpidd-primary init scripts. - Fix path substitution to use absolute paths. - Fix substitution to work on cmake and automake. - Set executable permissions for init scripts. Diffs - /trunk/qpid/cpp/etc/CMakeLists.txt 1466169 /trunk/qpid/cpp/etc/Makefile.am 1466169 /trunk/qpid/cpp/etc/qpidd-primary.in 1466169 /trunk/qpid/cpp/etc/qpidd.in 1466169 Diff: https://reviews.apache.org/r/10398/diff/ Testing --- make intstall, verify init scripts work. Thanks, Alan Conway
Re: Review Request: Provides the EventNotifier type, plus example agent and unit test.
On 2011-09-02 21:18:55, Steve Huston wrote: In the test program, isReadable won't sense a signaled Windows event handle. If the usage model for this feature is a select-able handle you will need a socket. What is the intended model? In the Windows code, the convention is to prefix system calls with :: (e.g., ::CreateEvent) Other areas of qpid that have OS-specific code keep it in a OS-specific directory (qpid/sys/windows) I used the HANDLE model rather than a socket due to previous discussions where that was the idea kicked around. Are there any resource hits on using a socket pair rather than a HANDLE for this mechanism? If not, I'll move it over. Thanks for the note on :: before WinAPI calls. I've added them. Under QMF there are no OS-specific directory packages. Are we interested in creating such a packaging under QMF? - Darryl --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1557/#review1739 --- On 2011-09-02 21:00:31, Darryl Pierce wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1557/ --- (Updated 2011-09-02 21:00:31) Review request for qpid, Kenneth Giusti, michael goulish, and Ted Ross. Summary --- Provides a new method for providing notification to an interested party when new messages are received. The EventNotifier class can be associated with either a console or agent session. The object provides a file descriptor which then becomes readable when there are messages to be processed. This implementation only supports Posix. There is some work necessary to get a Windows implementation in place. Diffs - trunk/qpid/cpp/bindings/qmf2/examples/cpp/Makefile.am 1164327 trunk/qpid/cpp/bindings/qmf2/examples/cpp/event_driven_list_agents.cpp PRE-CREATION trunk/qpid/cpp/include/qmf/AgentSession.h 1164327 trunk/qpid/cpp/include/qmf/ConsoleSession.h 1164327 trunk/qpid/cpp/include/qmf/PosixEventNotifier.h PRE-CREATION trunk/qpid/cpp/include/qmf/WindowsEventNotifer.h PRE-CREATION trunk/qpid/cpp/src/CMakeLists.txt 1164327 trunk/qpid/cpp/src/qmf.mk 1164327 trunk/qpid/cpp/src/qmf/AgentSession.cpp 1164327 trunk/qpid/cpp/src/qmf/AgentSessionImpl.h PRE-CREATION trunk/qpid/cpp/src/qmf/BaseEventNotifierImpl.h PRE-CREATION trunk/qpid/cpp/src/qmf/BaseEventNotifierImpl.cpp PRE-CREATION trunk/qpid/cpp/src/qmf/ConsoleSession.cpp 1164327 trunk/qpid/cpp/src/qmf/ConsoleSessionImpl.h 1164327 trunk/qpid/cpp/src/qmf/PosixEventNotifier.cpp PRE-CREATION trunk/qpid/cpp/src/qmf/PosixEventNotifierImpl.h PRE-CREATION trunk/qpid/cpp/src/qmf/PosixEventNotifierImpl.cpp PRE-CREATION trunk/qpid/cpp/src/qmf/WindowsEventNotifier.cpp PRE-CREATION trunk/qpid/cpp/src/qmf/WindowsEventNotifierImpl.h PRE-CREATION trunk/qpid/cpp/src/qmf/WindowsEventNotifierImpl.cpp PRE-CREATION trunk/qpid/cpp/src/tests/EventNotifierTest.cpp PRE-CREATION trunk/qpid/cpp/src/tests/Makefile.am 1164327 Diff: https://reviews.apache.org/r/1557/diff Testing --- An example agent takes the existing list_agents and uses an EventNotifier to respond to incoming messages rather than blocking on the ConsoleSession.nextReceiver() API. A unit test verifies that the file handle provides by the EventNotifier type is properly updating on incoming messgaes. Thanks, Darryl
Re: Review Request: Provides the EventNotifier type, plus example agent and unit test.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1557/ --- (Updated 2011-09-02 21:00:31.912847) Review request for qpid, Kenneth Giusti, michael goulish, and Ted Ross. Changes --- Updated with the initial Windows implementation. Summary --- Provides a new method for providing notification to an interested party when new messages are received. The EventNotifier class can be associated with either a console or agent session. The object provides a file descriptor which then becomes readable when there are messages to be processed. This implementation only supports Posix. There is some work necessary to get a Windows implementation in place. Diffs (updated) - trunk/qpid/cpp/bindings/qmf2/examples/cpp/Makefile.am 1164327 trunk/qpid/cpp/bindings/qmf2/examples/cpp/event_driven_list_agents.cpp PRE-CREATION trunk/qpid/cpp/include/qmf/AgentSession.h 1164327 trunk/qpid/cpp/include/qmf/ConsoleSession.h 1164327 trunk/qpid/cpp/include/qmf/PosixEventNotifier.h PRE-CREATION trunk/qpid/cpp/include/qmf/WindowsEventNotifer.h PRE-CREATION trunk/qpid/cpp/src/CMakeLists.txt 1164327 trunk/qpid/cpp/src/qmf.mk 1164327 trunk/qpid/cpp/src/qmf/AgentSession.cpp 1164327 trunk/qpid/cpp/src/qmf/AgentSessionImpl.h PRE-CREATION trunk/qpid/cpp/src/qmf/BaseEventNotifierImpl.h PRE-CREATION trunk/qpid/cpp/src/qmf/BaseEventNotifierImpl.cpp PRE-CREATION trunk/qpid/cpp/src/qmf/ConsoleSession.cpp 1164327 trunk/qpid/cpp/src/qmf/ConsoleSessionImpl.h 1164327 trunk/qpid/cpp/src/qmf/PosixEventNotifier.cpp PRE-CREATION trunk/qpid/cpp/src/qmf/PosixEventNotifierImpl.h PRE-CREATION trunk/qpid/cpp/src/qmf/PosixEventNotifierImpl.cpp PRE-CREATION trunk/qpid/cpp/src/qmf/WindowsEventNotifier.cpp PRE-CREATION trunk/qpid/cpp/src/qmf/WindowsEventNotifierImpl.h PRE-CREATION trunk/qpid/cpp/src/qmf/WindowsEventNotifierImpl.cpp PRE-CREATION trunk/qpid/cpp/src/tests/EventNotifierTest.cpp PRE-CREATION trunk/qpid/cpp/src/tests/Makefile.am 1164327 Diff: https://reviews.apache.org/r/1557/diff Testing --- An example agent takes the existing list_agents and uses an EventNotifier to respond to incoming messages rather than blocking on the ConsoleSession.nextReceiver() API. A unit test verifies that the file handle provides by the EventNotifier type is properly updating on incoming messgaes. Thanks, Darryl
Re: Review Request: Provides the EventNotifier type, plus example agent and unit test.
On 2011-08-19 21:47:12, Kenneth Giusti wrote: trunk/qpid/cpp/src/qmf/EventNotifierImpl.h, line 36 https://reviews.apache.org/r/1557/diff/1/?file=33112#file33112line36 May I make a suggestion - take a look at qpid::sys::IOHandle in the qpid code. This is an abstract class that hides the OS-specific bits (fd/Socket) pretty well. We could do something _like_ that here - add another level of abstraction by having getHandle() return a class instead of an 'int'. Or, perhaps not as pretty just typedef the return value using different OS-specific conditional compile: #if defined(_WIN32) #include whatever windoze headers typedef whatever windoze stuff IOHandle; #else typedef int IOHandle; #endif then we define: IOHandle getHandle() const; Andrew Stitcher wrote: The trouble is copying IOHandle can't help you! Ultimately providing an OS specific handle from an OS independent abstraction can't be done in an OS independent way. You always need to have a particular API with an OS specific signature to return the OS specific handle so you can use that handle together with your other application specific handling, at the very least the operation of returning an OS handle from an IOHandle is a non-portable part of the API. I suppose if that is the only non-portable part of the API then you're in a better shape than before, and it may be possible to do something clever(ish) with templates to avoid the actual header files being different so that might be a direction: Something like: template typename OSHandle OSHandle getOSHandle(IOHandle); and then define template int getOSHandleint(IOHandle); for unix and template HANDLE getOSHandleHANDLE(IOHandle); for Windows. - Completely untested thoughts - worth practically what you paid for them. There is a WSAPoll() API [1] that's available in Windows that takes a file handle ala poll(). [1] - http://msdn.microsoft.com/en-us/library/ms741669%28v=vs.85%29.aspx - Darryl --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1557/#review1569 --- On 2011-08-19 18:30:06, Darryl Pierce wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1557/ --- (Updated 2011-08-19 18:30:06) Review request for qpid, Kenneth Giusti, michael goulish, and Ted Ross. Summary --- Provides a new method for providing notification to an interested party when new messages are received. The EventNotifier class can be associated with either a console or agent session. The object provides a file descriptor which then becomes readable when there are messages to be processed. This implementation only supports Posix. There is some work necessary to get a Windows implementation in place. Diffs - trunk/qpid/cpp/bindings/qmf2/examples/cpp/Makefile.am 1159329 trunk/qpid/cpp/bindings/qmf2/examples/cpp/event_driven_list_agents.cpp PRE-CREATION trunk/qpid/cpp/include/qmf/AgentSession.h 1159329 trunk/qpid/cpp/include/qmf/ConsoleSession.h 1159329 trunk/qpid/cpp/include/qmf/EventNotifier.h PRE-CREATION trunk/qpid/cpp/src/CMakeLists.txt 1159329 trunk/qpid/cpp/src/qmf.mk 1159329 trunk/qpid/cpp/src/qmf/AgentSession.cpp 1159329 trunk/qpid/cpp/src/qmf/AgentSessionImpl.h PRE-CREATION trunk/qpid/cpp/src/qmf/ConsoleSession.cpp 1159329 trunk/qpid/cpp/src/qmf/ConsoleSessionImpl.h 1159329 trunk/qpid/cpp/src/qmf/EventNotifier.cpp PRE-CREATION trunk/qpid/cpp/src/qmf/EventNotifierImpl.h PRE-CREATION trunk/qpid/cpp/src/qmf/EventNotifierImpl.cpp PRE-CREATION trunk/qpid/cpp/src/qmf/PosixEventNotifierImpl.cpp PRE-CREATION trunk/qpid/cpp/src/tests/EventNotifierTest.cpp PRE-CREATION trunk/qpid/cpp/src/tests/Makefile.am 1159329 Diff: https://reviews.apache.org/r/1557/diff Testing --- An example agent takes the existing list_agents and uses an EventNotifier to respond to incoming messages rather than blocking on the ConsoleSession.nextReceiver() API. A unit test verifies that the file handle provides by the EventNotifier type is properly updating on incoming messgaes. Thanks, Darryl
Re: Review Request: Provides the EventNotifier type, plus example agent and unit test.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1557/ --- (Updated 2011-08-19 13:42:10.111642) Review request for qpid, Kenneth Giusti, michael goulish, and Ted Ross. Changes --- This is updated with feedback from tross. Summary --- Provides a new method for providing notification to an interested party when new messages are received. The EventNotifier class can be associated with either a console or agent session. The object provides a file descriptor which then becomes readable when there are messages to be processed. This implementation only supports Posix. There is some work necessary to get a Windows implementation in place. Diffs (updated) - trunk/qpid/cpp/bindings/qmf2/examples/cpp/Makefile.am 1159329 trunk/qpid/cpp/bindings/qmf2/examples/cpp/event_driven_list_agents.cpp PRE-CREATION trunk/qpid/cpp/include/qmf/AgentSession.h 1159329 trunk/qpid/cpp/include/qmf/ConsoleSession.h 1159329 trunk/qpid/cpp/include/qmf/EventNotifier.h PRE-CREATION trunk/qpid/cpp/src/CMakeLists.txt 1159329 trunk/qpid/cpp/src/qmf.mk 1159329 trunk/qpid/cpp/src/qmf/AgentSession.cpp 1159329 trunk/qpid/cpp/src/qmf/AgentSessionImpl.h PRE-CREATION trunk/qpid/cpp/src/qmf/ConsoleSession.cpp 1159329 trunk/qpid/cpp/src/qmf/ConsoleSessionImpl.h 1159329 trunk/qpid/cpp/src/qmf/EventNotifier.cpp PRE-CREATION trunk/qpid/cpp/src/qmf/EventNotifierImpl.h PRE-CREATION trunk/qpid/cpp/src/qmf/EventNotifierImpl.cpp PRE-CREATION trunk/qpid/cpp/src/qmf/PosixEventNotifierImpl.cpp PRE-CREATION trunk/qpid/cpp/src/tests/EventNotifierTest.cpp PRE-CREATION trunk/qpid/cpp/src/tests/Makefile.am 1159329 Diff: https://reviews.apache.org/r/1557/diff Testing --- An example agent takes the existing list_agents and uses an EventNotifier to respond to incoming messages rather than blocking on the ConsoleSession.nextReceiver() API. A unit test verifies that the file handle provides by the EventNotifier type is properly updating on incoming messgaes. Thanks, Darryl
Re: Review Request: Provides the EventNotifier type, plus example agent and unit test.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1557/ --- (Updated 2011-08-19 18:30:06.658895) Review request for qpid, Kenneth Giusti, michael goulish, and Ted Ross. Summary --- Provides a new method for providing notification to an interested party when new messages are received. The EventNotifier class can be associated with either a console or agent session. The object provides a file descriptor which then becomes readable when there are messages to be processed. This implementation only supports Posix. There is some work necessary to get a Windows implementation in place. Diffs (updated) - trunk/qpid/cpp/bindings/qmf2/examples/cpp/Makefile.am 1159329 trunk/qpid/cpp/bindings/qmf2/examples/cpp/event_driven_list_agents.cpp PRE-CREATION trunk/qpid/cpp/include/qmf/AgentSession.h 1159329 trunk/qpid/cpp/include/qmf/ConsoleSession.h 1159329 trunk/qpid/cpp/include/qmf/EventNotifier.h PRE-CREATION trunk/qpid/cpp/src/CMakeLists.txt 1159329 trunk/qpid/cpp/src/qmf.mk 1159329 trunk/qpid/cpp/src/qmf/AgentSession.cpp 1159329 trunk/qpid/cpp/src/qmf/AgentSessionImpl.h PRE-CREATION trunk/qpid/cpp/src/qmf/ConsoleSession.cpp 1159329 trunk/qpid/cpp/src/qmf/ConsoleSessionImpl.h 1159329 trunk/qpid/cpp/src/qmf/EventNotifier.cpp PRE-CREATION trunk/qpid/cpp/src/qmf/EventNotifierImpl.h PRE-CREATION trunk/qpid/cpp/src/qmf/EventNotifierImpl.cpp PRE-CREATION trunk/qpid/cpp/src/qmf/PosixEventNotifierImpl.cpp PRE-CREATION trunk/qpid/cpp/src/tests/EventNotifierTest.cpp PRE-CREATION trunk/qpid/cpp/src/tests/Makefile.am 1159329 Diff: https://reviews.apache.org/r/1557/diff Testing --- An example agent takes the existing list_agents and uses an EventNotifier to respond to incoming messages rather than blocking on the ConsoleSession.nextReceiver() API. A unit test verifies that the file handle provides by the EventNotifier type is properly updating on incoming messgaes. Thanks, Darryl
Re: Review Request: Provides the EventNotifier type, plus example agent and unit test.
On 2011-08-17 14:28:37, Ted Ross wrote: trunk/qpid/cpp/src/qmf/AgentSessionImpl.h, line 123 https://reviews.apache.org/r/1557/diff/1/?file=33108#file33108line123 This set of pointers is going to cause a memory leak when the AgentImpl is destroyed. It would be better to use a boost::shared_ptr instead of a raw pointer. Done. - Darryl --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1557/#review1495 --- On 2011-08-17 14:07:58, Darryl Pierce wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1557/ --- (Updated 2011-08-17 14:07:58) Review request for qpid, Kenneth Giusti, michael goulish, and Ted Ross. Summary --- Provides a new method for providing notification to an interested party when new messages are received. The EventNotifier class can be associated with either a console or agent session. The object provides a file descriptor which then becomes readable when there are messages to be processed. This implementation only supports Posix. There is some work necessary to get a Windows implementation in place. Diffs - trunk/qpid/cpp/bindings/qmf2/examples/cpp/Makefile.am 1158370 trunk/qpid/cpp/bindings/qmf2/examples/cpp/event_driven_list_agents.cpp PRE-CREATION trunk/qpid/cpp/include/qmf/AgentSession.h 1158370 trunk/qpid/cpp/include/qmf/ConsoleSession.h 1158370 trunk/qpid/cpp/include/qmf/EventNotifier.h PRE-CREATION trunk/qpid/cpp/src/CMakeLists.txt 1158370 trunk/qpid/cpp/src/qmf.mk 1158370 trunk/qpid/cpp/src/qmf/AgentSession.cpp 1158370 trunk/qpid/cpp/src/qmf/AgentSessionImpl.h PRE-CREATION trunk/qpid/cpp/src/qmf/ConsoleSession.cpp 1158370 trunk/qpid/cpp/src/qmf/ConsoleSessionImpl.h 1158370 trunk/qpid/cpp/src/qmf/EventNotifier.cpp PRE-CREATION trunk/qpid/cpp/src/qmf/EventNotifierImpl.h PRE-CREATION trunk/qpid/cpp/src/qmf/EventNotifierImpl.cpp PRE-CREATION trunk/qpid/cpp/src/qmf/PosixEventNotifierImpl.cpp PRE-CREATION trunk/qpid/cpp/src/tests/EventNotifierTest.cpp PRE-CREATION trunk/qpid/cpp/src/tests/Makefile.am 1158370 Diff: https://reviews.apache.org/r/1557/diff Testing --- An example agent takes the existing list_agents and uses an EventNotifier to respond to incoming messages rather than blocking on the ConsoleSession.nextReceiver() API. A unit test verifies that the file handle provides by the EventNotifier type is properly updating on incoming messgaes. Thanks, Darryl
Review Request: Provides the EventNotifier type, plus example agent and unit test.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1557/ --- Review request for qpid, Kenneth Giusti, michael goulish, and Ted Ross. Summary --- Provides a new method for providing notification to an interested party when new messages are received. The EventNotifier class can be associated with either a console or agent session. The object provides a file descriptor which then Diffs - trunk/qpid/cpp/bindings/qmf2/examples/cpp/Makefile.am 1158370 trunk/qpid/cpp/bindings/qmf2/examples/cpp/event_driven_list_agents.cpp PRE-CREATION trunk/qpid/cpp/include/qmf/AgentSession.h 1158370 trunk/qpid/cpp/include/qmf/ConsoleSession.h 1158370 trunk/qpid/cpp/include/qmf/EventNotifier.h PRE-CREATION trunk/qpid/cpp/src/CMakeLists.txt 1158370 trunk/qpid/cpp/src/qmf.mk 1158370 trunk/qpid/cpp/src/qmf/AgentSession.cpp 1158370 trunk/qpid/cpp/src/qmf/AgentSessionImpl.h PRE-CREATION trunk/qpid/cpp/src/qmf/ConsoleSession.cpp 1158370 trunk/qpid/cpp/src/qmf/ConsoleSessionImpl.h 1158370 trunk/qpid/cpp/src/qmf/EventNotifier.cpp PRE-CREATION trunk/qpid/cpp/src/qmf/EventNotifierImpl.h PRE-CREATION trunk/qpid/cpp/src/qmf/EventNotifierImpl.cpp PRE-CREATION trunk/qpid/cpp/src/qmf/PosixEventNotifierImpl.cpp PRE-CREATION trunk/qpid/cpp/src/tests/EventNotifierTest.cpp PRE-CREATION trunk/qpid/cpp/src/tests/Makefile.am 1158370 Diff: https://reviews.apache.org/r/1557/diff Testing --- An example agent takes the existing list_agents and uses an EventNotifier to respond to incoming messages rather than blocking on the ConsoleSession.nextReceiver() API. A unit test verifies that the file handle provides by the EventNotifier type is properly updating on incoming messgaes. Thanks, Darryl
Re: Review Request: Provides the EventNotifier type, plus example agent and unit test.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1557/ --- (Updated 2011-08-17 14:07:58.856191) Review request for qpid, Kenneth Giusti, michael goulish, and Ted Ross. Summary (updated) --- Provides a new method for providing notification to an interested party when new messages are received. The EventNotifier class can be associated with either a console or agent session. The object provides a file descriptor which then becomes readable when there are messages to be processed. This implementation only supports Posix. There is some work necessary to get a Windows implementation in place. Diffs - trunk/qpid/cpp/bindings/qmf2/examples/cpp/Makefile.am 1158370 trunk/qpid/cpp/bindings/qmf2/examples/cpp/event_driven_list_agents.cpp PRE-CREATION trunk/qpid/cpp/include/qmf/AgentSession.h 1158370 trunk/qpid/cpp/include/qmf/ConsoleSession.h 1158370 trunk/qpid/cpp/include/qmf/EventNotifier.h PRE-CREATION trunk/qpid/cpp/src/CMakeLists.txt 1158370 trunk/qpid/cpp/src/qmf.mk 1158370 trunk/qpid/cpp/src/qmf/AgentSession.cpp 1158370 trunk/qpid/cpp/src/qmf/AgentSessionImpl.h PRE-CREATION trunk/qpid/cpp/src/qmf/ConsoleSession.cpp 1158370 trunk/qpid/cpp/src/qmf/ConsoleSessionImpl.h 1158370 trunk/qpid/cpp/src/qmf/EventNotifier.cpp PRE-CREATION trunk/qpid/cpp/src/qmf/EventNotifierImpl.h PRE-CREATION trunk/qpid/cpp/src/qmf/EventNotifierImpl.cpp PRE-CREATION trunk/qpid/cpp/src/qmf/PosixEventNotifierImpl.cpp PRE-CREATION trunk/qpid/cpp/src/tests/EventNotifierTest.cpp PRE-CREATION trunk/qpid/cpp/src/tests/Makefile.am 1158370 Diff: https://reviews.apache.org/r/1557/diff Testing --- An example agent takes the existing list_agents and uses an EventNotifier to respond to incoming messages rather than blocking on the ConsoleSession.nextReceiver() API. A unit test verifies that the file handle provides by the EventNotifier type is properly updating on incoming messgaes. Thanks, Darryl
Re: Review Request: Provides the EventNotifier type, plus example agent and unit test.
On 2011-08-17 14:28:37, Ted Ross wrote: trunk/qpid/cpp/include/qmf/EventNotifier.h, line 40 https://reviews.apache.org/r/1557/diff/1/?file=33104#file33104line40 The session arguments should be passed as references, not pointers. Pointers can be null and you will have to deal with that case whereas references cannot be null and are generally cleaner and more elegant. The problem there is that the EventNotifier will have either a ConsoleSession or an AgentSession and not both, something will necessarily be null. That's why I chose a pointer instead of a reference. On 2011-08-17 14:28:37, Ted Ross wrote: trunk/qpid/cpp/src/qmf/AgentSessionImpl.h, line 90 https://reviews.apache.org/r/1557/diff/1/?file=33108#file33108line90 Again, please use references rather than pointers. Agreed here. I'll update the set and APIs on the Sessions to use a reference to an EventNotifier. On 2011-08-17 14:28:37, Ted Ross wrote: trunk/qpid/cpp/src/qmf/EventNotifierImpl.cpp, line 54 https://reviews.apache.org/r/1557/diff/1/?file=33113#file33113line54 What is this doing here? Sorry, leftover from debugging. I'll remove it. - Darryl --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1557/#review1495 --- On 2011-08-17 14:07:58, Darryl Pierce wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1557/ --- (Updated 2011-08-17 14:07:58) Review request for qpid, Kenneth Giusti, michael goulish, and Ted Ross. Summary --- Provides a new method for providing notification to an interested party when new messages are received. The EventNotifier class can be associated with either a console or agent session. The object provides a file descriptor which then becomes readable when there are messages to be processed. This implementation only supports Posix. There is some work necessary to get a Windows implementation in place. Diffs - trunk/qpid/cpp/bindings/qmf2/examples/cpp/Makefile.am 1158370 trunk/qpid/cpp/bindings/qmf2/examples/cpp/event_driven_list_agents.cpp PRE-CREATION trunk/qpid/cpp/include/qmf/AgentSession.h 1158370 trunk/qpid/cpp/include/qmf/ConsoleSession.h 1158370 trunk/qpid/cpp/include/qmf/EventNotifier.h PRE-CREATION trunk/qpid/cpp/src/CMakeLists.txt 1158370 trunk/qpid/cpp/src/qmf.mk 1158370 trunk/qpid/cpp/src/qmf/AgentSession.cpp 1158370 trunk/qpid/cpp/src/qmf/AgentSessionImpl.h PRE-CREATION trunk/qpid/cpp/src/qmf/ConsoleSession.cpp 1158370 trunk/qpid/cpp/src/qmf/ConsoleSessionImpl.h 1158370 trunk/qpid/cpp/src/qmf/EventNotifier.cpp PRE-CREATION trunk/qpid/cpp/src/qmf/EventNotifierImpl.h PRE-CREATION trunk/qpid/cpp/src/qmf/EventNotifierImpl.cpp PRE-CREATION trunk/qpid/cpp/src/qmf/PosixEventNotifierImpl.cpp PRE-CREATION trunk/qpid/cpp/src/tests/EventNotifierTest.cpp PRE-CREATION trunk/qpid/cpp/src/tests/Makefile.am 1158370 Diff: https://reviews.apache.org/r/1557/diff Testing --- An example agent takes the existing list_agents and uses an EventNotifier to respond to incoming messages rather than blocking on the ConsoleSession.nextReceiver() API. A unit test verifies that the file handle provides by the EventNotifier type is properly updating on incoming messgaes. Thanks, Darryl
Re: Review Request: Provides the EventNotifier type, plus example agent and unit test.
On 2011-08-17 14:28:37, Ted Ross wrote: trunk/qpid/cpp/include/qmf/EventNotifier.h, line 40 https://reviews.apache.org/r/1557/diff/1/?file=33104#file33104line40 The session arguments should be passed as references, not pointers. Pointers can be null and you will have to deal with that case whereas references cannot be null and are generally cleaner and more elegant. Darryl Pierce wrote: The problem there is that the EventNotifier will have either a ConsoleSession or an AgentSession and not both, something will necessarily be null. That's why I chose a pointer instead of a reference. Modified the code to take const references instead of pointers. The class no longer holds onto its Session instance as a result. - Darryl --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1557/#review1495 --- On 2011-08-17 14:07:58, Darryl Pierce wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1557/ --- (Updated 2011-08-17 14:07:58) Review request for qpid, Kenneth Giusti, michael goulish, and Ted Ross. Summary --- Provides a new method for providing notification to an interested party when new messages are received. The EventNotifier class can be associated with either a console or agent session. The object provides a file descriptor which then becomes readable when there are messages to be processed. This implementation only supports Posix. There is some work necessary to get a Windows implementation in place. Diffs - trunk/qpid/cpp/bindings/qmf2/examples/cpp/Makefile.am 1158370 trunk/qpid/cpp/bindings/qmf2/examples/cpp/event_driven_list_agents.cpp PRE-CREATION trunk/qpid/cpp/include/qmf/AgentSession.h 1158370 trunk/qpid/cpp/include/qmf/ConsoleSession.h 1158370 trunk/qpid/cpp/include/qmf/EventNotifier.h PRE-CREATION trunk/qpid/cpp/src/CMakeLists.txt 1158370 trunk/qpid/cpp/src/qmf.mk 1158370 trunk/qpid/cpp/src/qmf/AgentSession.cpp 1158370 trunk/qpid/cpp/src/qmf/AgentSessionImpl.h PRE-CREATION trunk/qpid/cpp/src/qmf/ConsoleSession.cpp 1158370 trunk/qpid/cpp/src/qmf/ConsoleSessionImpl.h 1158370 trunk/qpid/cpp/src/qmf/EventNotifier.cpp PRE-CREATION trunk/qpid/cpp/src/qmf/EventNotifierImpl.h PRE-CREATION trunk/qpid/cpp/src/qmf/EventNotifierImpl.cpp PRE-CREATION trunk/qpid/cpp/src/qmf/PosixEventNotifierImpl.cpp PRE-CREATION trunk/qpid/cpp/src/tests/EventNotifierTest.cpp PRE-CREATION trunk/qpid/cpp/src/tests/Makefile.am 1158370 Diff: https://reviews.apache.org/r/1557/diff Testing --- An example agent takes the existing list_agents and uses an EventNotifier to respond to incoming messages rather than blocking on the ConsoleSession.nextReceiver() API. A unit test verifies that the file handle provides by the EventNotifier type is properly updating on incoming messgaes. Thanks, Darryl
Re: Review Request: Provides the EventNotifier type, plus example agent and unit test.
On 2011-08-17 16:34:38, Ted Ross wrote: trunk/qpid/cpp/bindings/qmf2/examples/cpp/event_driven_list_agents.cpp, line 83 https://reviews.apache.org/r/1557/diff/1/?file=33101#file33101line83 When using the notifier to control your program's main loop, nextEvent should be called with a timeout of Duration::IMMEDIATE so that it cannot block. Done. - Darryl --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1557/#review1501 --- On 2011-08-17 14:07:58, Darryl Pierce wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1557/ --- (Updated 2011-08-17 14:07:58) Review request for qpid, Kenneth Giusti, michael goulish, and Ted Ross. Summary --- Provides a new method for providing notification to an interested party when new messages are received. The EventNotifier class can be associated with either a console or agent session. The object provides a file descriptor which then becomes readable when there are messages to be processed. This implementation only supports Posix. There is some work necessary to get a Windows implementation in place. Diffs - trunk/qpid/cpp/bindings/qmf2/examples/cpp/Makefile.am 1158370 trunk/qpid/cpp/bindings/qmf2/examples/cpp/event_driven_list_agents.cpp PRE-CREATION trunk/qpid/cpp/include/qmf/AgentSession.h 1158370 trunk/qpid/cpp/include/qmf/ConsoleSession.h 1158370 trunk/qpid/cpp/include/qmf/EventNotifier.h PRE-CREATION trunk/qpid/cpp/src/CMakeLists.txt 1158370 trunk/qpid/cpp/src/qmf.mk 1158370 trunk/qpid/cpp/src/qmf/AgentSession.cpp 1158370 trunk/qpid/cpp/src/qmf/AgentSessionImpl.h PRE-CREATION trunk/qpid/cpp/src/qmf/ConsoleSession.cpp 1158370 trunk/qpid/cpp/src/qmf/ConsoleSessionImpl.h 1158370 trunk/qpid/cpp/src/qmf/EventNotifier.cpp PRE-CREATION trunk/qpid/cpp/src/qmf/EventNotifierImpl.h PRE-CREATION trunk/qpid/cpp/src/qmf/EventNotifierImpl.cpp PRE-CREATION trunk/qpid/cpp/src/qmf/PosixEventNotifierImpl.cpp PRE-CREATION trunk/qpid/cpp/src/tests/EventNotifierTest.cpp PRE-CREATION trunk/qpid/cpp/src/tests/Makefile.am 1158370 Diff: https://reviews.apache.org/r/1557/diff Testing --- An example agent takes the existing list_agents and uses an EventNotifier to respond to incoming messages rather than blocking on the ConsoleSession.nextReceiver() API. A unit test verifies that the file handle provides by the EventNotifier type is properly updating on incoming messgaes. Thanks, Darryl
Re: Review Request: Provides the EventNotifier type, plus example agent and unit test.
On 2011-08-17 16:34:38, Ted Ross wrote:
trunk/qpid/cpp/src/qmf/ConsoleSessionImpl.h, line 110
https://reviews.apache.org/r/1557/diff/1/?file=33110#file33110line110
You need to pay attention to mutex protection of eventNotifiers.
There's a convention used (see enqueueEvent and enqueueEventLH) whereby
functions that assume that a lock is held have their names suffixed by LH
(lock held). alertEventNotifiers is (I claim) always going to be called
with the session lock held and should be a LH function.
{add,remove}EventNotifier are not LH functions but they must acquire
the lock before touching eventNotifiers. Not doing so will cause
difficult-to-debug problems down the road.
Both are done:
* renamed alertEventNotifiers() to alertEventNotifiersLH() and updated all
affected code, and
* added qpid::sys::Mutex::ScopedLocal to {ad,remove}EventNotifier() methods in
both Session types.
- Darryl
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/1557/#review1501
---
On 2011-08-17 14:07:58, Darryl Pierce wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/1557/
---
(Updated 2011-08-17 14:07:58)
Review request for qpid, Kenneth Giusti, michael goulish, and Ted Ross.
Summary
---
Provides a new method for providing notification to an interested party when
new messages are received.
The EventNotifier class can be associated with either a console or agent
session. The object provides a file descriptor which then becomes readable
when there are messages to be processed.
This implementation only supports Posix. There is some work necessary to get
a Windows implementation in place.
Diffs
-
trunk/qpid/cpp/bindings/qmf2/examples/cpp/Makefile.am 1158370
trunk/qpid/cpp/bindings/qmf2/examples/cpp/event_driven_list_agents.cpp
PRE-CREATION
trunk/qpid/cpp/include/qmf/AgentSession.h 1158370
trunk/qpid/cpp/include/qmf/ConsoleSession.h 1158370
trunk/qpid/cpp/include/qmf/EventNotifier.h PRE-CREATION
trunk/qpid/cpp/src/CMakeLists.txt 1158370
trunk/qpid/cpp/src/qmf.mk 1158370
trunk/qpid/cpp/src/qmf/AgentSession.cpp 1158370
trunk/qpid/cpp/src/qmf/AgentSessionImpl.h PRE-CREATION
trunk/qpid/cpp/src/qmf/ConsoleSession.cpp 1158370
trunk/qpid/cpp/src/qmf/ConsoleSessionImpl.h 1158370
trunk/qpid/cpp/src/qmf/EventNotifier.cpp PRE-CREATION
trunk/qpid/cpp/src/qmf/EventNotifierImpl.h PRE-CREATION
trunk/qpid/cpp/src/qmf/EventNotifierImpl.cpp PRE-CREATION
trunk/qpid/cpp/src/qmf/PosixEventNotifierImpl.cpp PRE-CREATION
trunk/qpid/cpp/src/tests/EventNotifierTest.cpp PRE-CREATION
trunk/qpid/cpp/src/tests/Makefile.am 1158370
Diff: https://reviews.apache.org/r/1557/diff
Testing
---
An example agent takes the existing list_agents and uses an EventNotifier to
respond to incoming messages rather than blocking on the
ConsoleSession.nextReceiver() API.
A unit test verifies that the file handle provides by the EventNotifier type
is properly updating on incoming messgaes.
Thanks,
Darryl
Re: Review Request: Provides the EventNotifier type, plus example agent and unit test.
On 2011-08-17 16:34:38, Ted Ross wrote: trunk/qpid/cpp/src/qmf/AgentSession.cpp, line 1095 https://reviews.apache.org/r/1557/diff/1/?file=33107#file33107line1095 The logic here is incorrect. It is not necessarily the case that the reception of a message from the AMQP session will result in the creation of an event in the QMF session. Conversely, there are cases where QMF events will be generated when there were no AMQP messages received (e.g. Agent age-out due to lack of heartbeats received). This is not the right place to kick the notifiers. The notifier should be enabled when the event queue goes from empty to non-empty and should be disabled when the event queue becomes empty. I've moved the call to alertEventNotifiersLH() to the enqueueEventLH() method in both Session types. There also needs to be a spot where an event should fire when the event queue is empty. I've placed a call within nextEvent() that does that both after popping the next message off of the queue and, if the queue is empty, in an else statement. On 2011-08-17 16:34:38, Ted Ross wrote: trunk/qpid/cpp/src/qmf/PosixEventNotifierImpl.cpp, line 36 https://reviews.apache.org/r/1557/diff/1/?file=33114#file33114line36 We've had a problem in the past with a similar pattern: If the pipe ever fills up (the capacity is OS-specific), the write call may misbehave, causing deadlocks. Also, if the pipe ever has more than BUFFER_SIZE characters in it, setting readable to False will have no effect. Here's a good test case that will fail: call setReadable(true) 11 times; call setReadable(false) once; Verify that the fd is unreadable. I suggest keeping a boolean state that tracks the readability of the socket. If the call to setReadable will not result in a state change, don't do anything. This guarantees that the pipe will never have more than one character in it at any time. Since this call is always invoked under the session's lock, and a notifier can have exactly one session, there is no need to add a mutex to protect the stored state. Done. Now the method will check: * if we're readable and the new state is readable==false then pull the byte out, else * if we're not readable and the new state is readable=true then write a byte - Darryl --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1557/#review1501 --- On 2011-08-17 14:07:58, Darryl Pierce wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1557/ --- (Updated 2011-08-17 14:07:58) Review request for qpid, Kenneth Giusti, michael goulish, and Ted Ross. Summary --- Provides a new method for providing notification to an interested party when new messages are received. The EventNotifier class can be associated with either a console or agent session. The object provides a file descriptor which then becomes readable when there are messages to be processed. This implementation only supports Posix. There is some work necessary to get a Windows implementation in place. Diffs - trunk/qpid/cpp/bindings/qmf2/examples/cpp/Makefile.am 1158370 trunk/qpid/cpp/bindings/qmf2/examples/cpp/event_driven_list_agents.cpp PRE-CREATION trunk/qpid/cpp/include/qmf/AgentSession.h 1158370 trunk/qpid/cpp/include/qmf/ConsoleSession.h 1158370 trunk/qpid/cpp/include/qmf/EventNotifier.h PRE-CREATION trunk/qpid/cpp/src/CMakeLists.txt 1158370 trunk/qpid/cpp/src/qmf.mk 1158370 trunk/qpid/cpp/src/qmf/AgentSession.cpp 1158370 trunk/qpid/cpp/src/qmf/AgentSessionImpl.h PRE-CREATION trunk/qpid/cpp/src/qmf/ConsoleSession.cpp 1158370 trunk/qpid/cpp/src/qmf/ConsoleSessionImpl.h 1158370 trunk/qpid/cpp/src/qmf/EventNotifier.cpp PRE-CREATION trunk/qpid/cpp/src/qmf/EventNotifierImpl.h PRE-CREATION trunk/qpid/cpp/src/qmf/EventNotifierImpl.cpp PRE-CREATION trunk/qpid/cpp/src/qmf/PosixEventNotifierImpl.cpp PRE-CREATION trunk/qpid/cpp/src/tests/EventNotifierTest.cpp PRE-CREATION trunk/qpid/cpp/src/tests/Makefile.am 1158370 Diff: https://reviews.apache.org/r/1557/diff Testing --- An example agent takes the existing list_agents and uses an EventNotifier to respond to incoming messages rather than blocking on the ConsoleSession.nextReceiver() API. A unit test verifies that the file handle provides by the EventNotifier type is properly updating on incoming messgaes. Thanks, Darryl
