[
https://issues.apache.org/jira/browse/PROTON-262?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13594571#comment-13594571
]
Keith Wall commented on PROTON-262:
-----------------------------------
Hi Rob,
A couple of comments:
1) JNIConnection, JNISession, and JNILink#getRemoteCondition: The code to set
the _remoteCondition is duplicated across three classes. This should be
extracted to a utility class.
_remoteCondition.setCondition(Symbol.valueOf(Proton.pn_condition_get_name(cond)));
_remoteCondition.setDescription(Proton.pn_condition_get_description(cond));
JNIData data = new JNIData(Proton.pn_condition_info(cond));
if(data.next() == Data.DataType.MAP)
{
_remoteCondition.setInfo(data.getJavaMap());
}
2) Whilst I see that
http://docs.oasis-open.org/amqp/core/v1.0/os/amqp-core-transport-v1.0-os.html#type-error
defines condition info as a map, the fact that the implementation will
silently skip a JNIData returning a non-map type, or ignore additional values
gives concern. I'd prefer to see an IllegalStateException in these unexpected
scenarios to guarantee we are alerted to a misbehaving implementation.
if(data.next() == Data.DataType.MAP)
{
_remoteCondition.setInfo(data.getJavaMap());
}
// what if non-map type
// what if data yields more values
3) JNIConnection, JNISession, and JNILink#close(): The code to transfer the
local condition to the underlying proton-c api ( pn_condition_set_name,
pn_condition_set_description) is duplicated across three classes. This should
be extracted to a utility class.
4) What is the correct way for an application to determine if an ErrorCondition
is populated? Would it not be more explicit if ErrorCondition had a method
such as isSet() rather than forcing the caller to use #getCondition() != null?
5) It appears that if an application were to call, say,
Connection#setCondition(), multiple times, it is only the last error that goes
over the wire on the closure of the endpoint. I think this is reasonable, but
the Javadoc on Endpoint#setCondition should note this fact.
6) ErrorCondition#_info becomes a reference to an application owned map via
#setInfo, then lets this mutable state escape by #getInfo. I think the
implementation would be more robust if ErrorCondition copied the incoming map
on setInfo, and returned a immutable wrapper on calls #getInfo. Also to
simplify client coding, if ErrorCondition#getInfo returned an emptyMap() rather
than null.
7) EndpointError has been renamed ErrorCondition. What is the Proton's policy
on deprecation?
regards, Keith.
> Fully implement (error) condtion API in Proton-J
> ------------------------------------------------
>
> Key: PROTON-262
> URL: https://issues.apache.org/jira/browse/PROTON-262
> Project: Qpid Proton
> Issue Type: Bug
> Components: proton-j
> Reporter: Rob Godfrey
> Assignee: Keith Wall
> Fix For: 0.5
>
>
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira
