qpid-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Kenneth Giusti" <kgiu...@apache.org>
Subject Re: Review Request 36992: PROTON-886 and PROTON-930 -- handle_max and AMQP numeric default constants
Date Fri, 31 Jul 2015 20:27:31 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36992/#review93765
-----------------------------------------------------------



proton-c/bindings/python/proton/__init__.py (line 2571)
<https://reviews.apache.org/r/36992/#comment148168>

    You can make 'handle_max' a read only property simply by specifying:
    
    @property
    def handle_max(self):
        return pn_session_get_handle_max(self._impl)
    
    instead of defining a dummy set method _set_handle_max.
    
    I'll make you into a Python expert in no time.  Yes, that is a threat.



proton-c/include/proton/session.h (line 313)
<https://reviews.apache.org/r/36992/#comment148169>

    Actually, it isn't finalized until the session has opened, i.e. pn_session_state(session)
== PN_REMOTE_ACTIVE | PN_LOCAL_ACTIVE,  which means the BEGIN was sent and a remote BEGIN
was received.



proton-c/src/engine/engine-internal.h (line 116)
<https://reviews.apache.org/r/36992/#comment148178>

    nuke this



proton-c/src/engine/engine.c (line 1004)
<https://reviews.apache.org/r/36992/#comment148175>

    Not necessary - see below:



proton-c/src/engine/engine.c (line 2227)
<https://reviews.apache.org/r/36992/#comment148177>

    To check if this session has already sent a BEGIN:
    
    if (!(session->endpoint.state & PN_LOCAL_UNINIT)) {
    .....
    
    we shouldn't be adding yet more state flags to the session - it's confusing enough as
it is.  Given the bitmask check isn't as self-documenting (thanks, Rafi!), it's pretty much
the way it's done all over the code.



proton-c/src/engine/engine.c (line 2230)
<https://reviews.apache.org/r/36992/#comment148181>

    use pn_min() in utils.h



proton-c/src/protocol.h.py (line 39)
<https://reviews.apache.org/r/36992/#comment148172>

    Yay! another chance to make you a Python programmer!
    
    You don't need the standalone int conversion statement to test the type (it's not 'natural'
python):
    
    int(default)
    
    Use '%d' as the format flag (instead of %s), and pass it the int(default) in the print()
statement.



proton-c/src/transport/transport.c (line 1219)
<https://reviews.apache.org/r/36992/#comment148180>

    use pn_min() in utils.h



proton-c/src/transport/transport.c (line 2007)
<https://reviews.apache.org/r/36992/#comment148179>

    nuke me


- Kenneth Giusti


On July 31, 2015, 4:56 p.m., michael goulish wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36992/
> -----------------------------------------------------------
> 
> (Updated July 31, 2015, 4:56 p.m.)
> 
> 
> Review request for qpid, Kenneth Giusti and Ted Ross.
> 
> 
> Repository: qpid-proton-git
> 
> 
> Description
> -------
> 
> PROTON-886 and PROTON-930 -- handle_max and AMQP numeric default constants.  
> 
>     Note:
>     {
>       I am skipping the java implementation for now,
>       because I would like to get the C version out of
>       my life.  (And this diff is already quite large
>       enough, I think.)
> 
>       I will write a separate JIRA for the java version
>       and make it refer to this one.  I will work on it
>       as a background task.
>     }
> 
>     This diff adds two pieces of functionality:
> 
>       1. extract numeric default valuies from AMQP
>          xml files and store them as defined constants
>          in protocol.h  (PROTON_930)
> 
>       2. add code to enforce AMQP 1.0 spec mandated
>          behavior with respect to handle-max values
>          -- i.e. negotiated limits on number of links --
>          per session.  (PROTON-886)
> 
>     These two changes are combined into one checkin
>     because of some earlier feedback I got suggesting
>     that I not check in PROTON-930 until I had some
>     code that could actually use the constants that it
>     creates.
> 
>     The code that is generated by the changes in
>     proton-c/src/protocol.h.py show up in the file
>     protocol.h.  It is this:
> 
>     --------- begin snippet -----------------------
>     /* Numeric default values */
>     #define AMQP_OPEN_MAX_FRAME_SIZE_DEFAULT 4294967295
>     #define AMQP_OPEN_CHANNEL_MAX_DEFAULT 65535
>     #define AMQP_BEGIN_HANDLE_MAX_DEFAULT 4294967295
>     #define AMQP_SOURCE_TIMEOUT_DEFAULT 0
>     #define AMQP_TARGET_TIMEOUT_DEFAULT 0
>     --------- end snippet -----------------------
> 
> 
> Diffs
> -----
> 
>   proton-c/bindings/python/proton/__init__.py 46b9466 
>   proton-c/include/proton/session.h 94d2869 
>   proton-c/src/engine/engine-internal.h 727f50d 
>   proton-c/src/engine/engine.c 9043e0b 
>   proton-c/src/protocol.h.py bbc0dfe 
>   proton-c/src/transport/transport.c 6abf862 
>   tests/python/proton_tests/engine.py 7a1d539 
> 
> Diff: https://reviews.apache.org/r/36992/diff/
> 
> 
> Testing
> -------
> 
> ctest -VV with Java tests running.
> 
> 
> Thanks,
> 
> michael goulish
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message