gemmellr commented on a change in pull request #3079: ARTEMIS-2706 Use
FrameSize to decide when to flush large messages
URL: https://github.com/apache/activemq-artemis/pull/3079#discussion_r408038172
##########
File path:
artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/proton/ProtonServerSenderContext.java
##########
@@ -862,7 +862,9 @@ void deliver() {
sender.send(buf.array(), 0, size);
- connection.instantFlush();
+ if (protonSession.session.getOutgoingBytes() >=
protonSession.session.getConnection().getTransport().getOutboundFrameSizeLimit())
{
+ connection.instantFlush();
+ }
Review comment:
I dont think this makes the most sense as there isnt anything to say there
arent bytes for other messages in the session, so using that to govern
behaviour for a particular delivery isnt a reliable way to achieve the desired
behaviour.
The delivery has its own buffer, using that to govern behaviour would seem
the most appropriate, see Delivery.pending() and available() (which both do the
same thing hehe, https://issues.apache.org/jira/browse/PROTON-1409).
Just checking the brokers outbound size limit doesnt allow for the fact the
receivers frame size limit may be lower than it, so it may be worth calculating
the lower of the two up front, otherwise it may never flush here. Similarly,
the return slightly higher up may mean it returns without flushing following a
change like this.
The existing flush here will presumably also be why ARTEMIS-2707 occurred,
since the delivery isnt indicated complete (sender.advance()) or settled (which
implicitly completes) until further down just before another flush, yet would
have already had all its payload sent at this point. With the addition of an
'if outstanding is over frame size' guard here it would become less certain
whether the flush and subsquent empty transfer occurred however, depending then
on whether the threshhold was exceeded after the final send.
I'd note that by creating such a small buffer, you are forcing a similarly
small allocation on every send call as the payload is copied into the delivery.
Using a bigger buffer would seem sensible when appropriate. Perhaps the message
size can be discerned and something suitable selected based on that and the
frame sizing. Or also using NettyReadable and sendNoCopy to avoid the copy
(note though that per its API, sendNoCopy can/will still copy if there is
outstanding data from a prior send call, hence passing a more appropriately
sized buffer makes sense there too).
In terms of sizing on send, it may be worth allowing for some room for the
frame header and transfer encoding, such that by supplying a little less than
[a multiple of] the max you end up avoiding emitting a 'full' frame followed by
a tiny one just to complete sending some small remaining content, then another
full one and tiny one for subsequent send call content.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
