Re: Review Request 36992: PROTON-886 and PROTON-930 -- handle_max and AMQP numeric default constants
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36992/
---
(Updated Aug. 5, 2015, 9:52 a.m.)
Review request for qpid, Kenneth Giusti and Ted Ross.
Changes
---
Improvements from kgiusti's feedback.
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 (updated)
-
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
Re: Review Request 36992: PROTON-886 and PROTON-930 -- handle_max and AMQP numeric default constants
On July 31, 2015, 8:27 p.m., Kenneth Giusti wrote:
proton-c/src/engine/engine.c, line 2230
https://reviews.apache.org/r/36992/diff/1/?file=1026182#file1026182line2230
use pn_min() in utils.h
Wow we ... have a min macro. I... That'sOK! Absolutely.
On July 31, 2015, 8:27 p.m., Kenneth Giusti wrote:
proton-c/src/protocol.h.py, line 39
https://reviews.apache.org/r/36992/diff/1/?file=1026183#file1026183line39
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.
Bah. What's 'natural'? standalone makes the intent more clear. Now I'm
putting a comment there. Next they'll be telling me how to use whitespace.
Oh, wait...
- michael
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36992/#review93765
---
On Aug. 5, 2015, 9:52 a.m., michael goulish wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36992/
---
(Updated Aug. 5, 2015, 9:52 a.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
Re: Review Request 36992: PROTON-886 and PROTON-930 -- handle_max and AMQP numeric default constants
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36992/#review94317
---
Ship it!
Ship It!
- Kenneth Giusti
On Aug. 5, 2015, 9:52 a.m., michael goulish wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36992/
---
(Updated Aug. 5, 2015, 9:52 a.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
Review Request 36992: PROTON-886 and PROTON-930 -- handle_max and AMQP numeric default constants
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36992/
---
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
Re: Review Request 36992: PROTON-886 and PROTON-930 -- handle_max and AMQP numeric default constants
---
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
