astitcher commented on a change in pull request #354:
URL: https://github.com/apache/qpid-proton/pull/354#discussion_r796829418
##########
File path: python/proton/_handlers.py
##########
@@ -663,11 +667,19 @@ class MessagingHandler(Handler, Acking):
simpler to deal with and/or avoids repetitive tasks for common use
cases.
+ .. note:: Sender **auto-settlement** only occurs for a delivery after the
+ sender receives a settled disposition for that delivery. Otherwise,
+ there would be no way to receive any further events for that delivery
+ (such as the subsequent on_settle message that might be expected when
+ the receiver finally settles the message).
+
Review comment:
Putting this comment here seems unmotivated. It's just not connected
narratively with the comment before.
##########
File path: python/proton/_handlers.py
##########
@@ -950,11 +962,19 @@ class TransactionalClientHandler(MessagingHandler,
TransactionHandler):
and provides a convenience method :meth:`accept` for performing
a transactional acceptance of received messages.
+ .. note:: Sender **auto-settlement** only occurs for a delivery after the
+ sender receives a settled disposition for that delivery. Otherwise,
+ there would be no way to receive any further events for that delivery
+ (such as the subsequent on_settle message that might be expected when
+ the receiver finally settles the message).
+
:param prefetch: Initial flow credit for receiving messages, defaults to
10.
- :param auto_accept: If ``True``, accept all messages (default). Otherwise
messages
- must be individually accepted or rejected.
+ :param auto_accept: If ``True``, accept all messages (default). Otherwise
+ messages must be individually accepted or rejected.
:param auto_settle: If ``True``, settle all messages (default). Otherwise
- messages must be explicitly settled.
+ messages must be explicitly settled. Sender auto-settlement only occurs
+ for a delivery after the sender receives a settled disposition for that
+ delivery.
Review comment:
Again use the previous suggestion.
##########
File path: python/proton/_handlers.py
##########
@@ -50,6 +50,10 @@ class OutgoingMessageHandler(Handler):
:param auto_settle: If ``True``, settle all messages (default). Otherwise
messages must be explicitly settled.
+
+ .. note:: Sender auto-settlement only occurs for a delivery after the
Review comment:
Delete this as a separate para add the comment into the basic
description of auto settle behaviour as in:
If ``True`` (default), automatically settle messages upon receiving a
settled disposition for that delivery...
##########
File path: cpp/include/proton/sender_options.hpp
##########
@@ -82,7 +82,16 @@ class sender_options {
/// Set the delivery mode on the sender.
PN_CPP_EXTERN sender_options& delivery_mode(delivery_mode);
- /// Automatically settle messages (default is true).
+ /**
+ * Automatically settle messages (default is true).
+ *
+ * \note
+ * Sender **auto-settlement** only occurs for a delivery after the
+ * sender receives a settled disposition for that delivery. Otherwise,
+ * there would be no way to receive any further events for that delivery
+ * (such as the subsequent on_settle message that might be expected when
+ * the receiver finally settles the message).
+ */
Review comment:
I think the suggested wording I made for the python API is good here too
(see below). I don't see the need to call out auto-settlement in so many words,
just explain the basic behaviour as below.
In the API docs that explain settlement we should explain that you can
receive no more events after settling a delivery, but that understanding is
fundamental to what *settlement is* and so shouldn't need explaining every
place we talk about settlement.
##########
File path: ruby/lib/core/sender.rb
##########
@@ -29,10 +29,17 @@ class Sender < Link
# Open the {Sender} link
#
+ # @note Sender **auto-settlement** only occurs for a delivery after the
+ # sender receives a settled disposition for that delivery. Otherwise,
+ # there would be no way to receive any further events for that delivery
+ # (such as the subsequent on_settle message that might be expected when
+ # the receiver finally settles the message).
Review comment:
I honestly don't understand why auto-settlement is called out specially
here (or above). There's nothing special about it. Its described below under
its option.
##########
File path: c/include/proton/delivery.h
##########
@@ -287,6 +287,12 @@ PN_EXTERN void pn_delivery_abort(pn_delivery_t *delivery);
* @note If pn_delivery_current(delivery) is true before the call then
* pn_link_advance(pn_delivery_link(deliver)) is called automatically.
*
+ * @note The sender **should not** settle after only receiving a terminal
+ * status disposition with no settle flag, as then there would then be no way
+ * to receive any further events for that delivery (such as the subsequent
+ * on_settle message that might be expected when the receiver finally settles
+ * the message).
+ *
Review comment:
What does settlement have to do with delivery abort? If there is a
connection it needs to be called out explicitly to explain why this comment is
here. Afaict If the sender aborts a delivery then it's likely they actually
*do* want to settle the delivery because the delivery is now effectively dead
and why would they want any more events about it? If you are making a comment
relevant to this point then say that explicitly.
Essentially I think **should not** is way too strong here - if the
application has no need for the receive settlement after aborting the message,
and it seems to me likely they don't, then settling the delivery at the sender
end first should be fine.
##########
File path: ruby/lib/core/sender.rb
##########
@@ -29,10 +29,17 @@ class Sender < Link
# Open the {Sender} link
#
+ # @note Sender **auto-settlement** only occurs for a delivery after the
+ # sender receives a settled disposition for that delivery. Otherwise,
+ # there would be no way to receive any further events for that delivery
+ # (such as the subsequent on_settle message that might be expected when
+ # the receiver finally settles the message).
# @overload open_sender(address)
# @param address [String] address of the target to send to
# @overload open_sender(opts)
- # @option opts [Boolean] :auto_settle (true) if true, automatically
settle transfers
+ # @option opts [Boolean] :auto_settle (true) if true, automatically
settle transfers.
+ # **Note:** Sender auto-settlement only occurs for a delivery after
the sender receives
+ # a settled disposition for that delivery.
Review comment:
As above we're just describing the basic behaviour, do inline as I
suggested for python.
##########
File path: python/proton/_handlers.py
##########
@@ -663,11 +667,19 @@ class MessagingHandler(Handler, Acking):
simpler to deal with and/or avoids repetitive tasks for common use
cases.
+ .. note:: Sender **auto-settlement** only occurs for a delivery after the
+ sender receives a settled disposition for that delivery. Otherwise,
+ there would be no way to receive any further events for that delivery
+ (such as the subsequent on_settle message that might be expected when
+ the receiver finally settles the message).
+
:param prefetch: Initial flow credit for receiving messages, defaults to
10.
:param auto_accept: If ``True``, accept all messages (default). Otherwise
messages must be individually accepted or rejected.
:param auto_settle: If ``True``, settle all messages (default). Otherwise
- messages must be explicitly settled.
+ messages must be explicitly settled. Sender auto-settlement only occurs
+ for a delivery after the sender receives a settled disposition for that
+ delivery.
Review comment:
Use the suggested wording as above. We are describing the basic
behaviour here and there is no need for a confusing qualification IMO.
##########
File path: python/proton/_handlers.py
##########
@@ -950,11 +962,19 @@ class TransactionalClientHandler(MessagingHandler,
TransactionHandler):
and provides a convenience method :meth:`accept` for performing
a transactional acceptance of received messages.
+ .. note:: Sender **auto-settlement** only occurs for a delivery after the
+ sender receives a settled disposition for that delivery. Otherwise,
+ there would be no way to receive any further events for that delivery
+ (such as the subsequent on_settle message that might be expected when
+ the receiver finally settles the message).
+
Review comment:
As above delete this para we cover it below under auto settlement.
If we want to explain this point we need to attach the explanation to the
explanation of settlement as a concept,.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
